-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: prettier run on newText #353
base: main
Are you sure you want to change the base?
fix: prettier run on newText #353
Conversation
🦋 Changeset detectedLatest commit: 3db3cfe The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
apps/cli/src/bin.ts
Outdated
@@ -110,7 +116,11 @@ const writeToFiles = async (uriArray: string[], { cwd }: { cwd: string }) => { | |||
|
|||
const edits = getTsTypesEdits(types); | |||
if (edits.length > 0) { | |||
const newFile = processFileEdits(fileContents, edits); | |||
const newFile = await prettify( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I'd like to remove the dependency on Prettier altogether. I'd much more prefer to detect the preferred quotes style and just use that within the edit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplest (and hackiest) way I can think of is a check in newFile for number of double quotes vs. number of single quotes and go with whichever is more common. That would remove requirement for prettier, but would be quite ugly.
If prettier was kept, we could check for quotes by using something like:
const quote = (await prettier.resolveConfig(cwd)).singleQuote ? "'" : '"';
This all has a downside of ignoring non-quote prettier features that we may violate like line length and trailing commas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's rather a safe bet to assume that there is some import statement in the file and we could piggy-back our quotes detection based on that with smth like /import\s(.|\n)*(['"])/
.
This all has a downside of ignoring non-quote prettier features that we may violate like line length and trailing commas.
I understand the concern but we are not quite in the code formatting business. There are multiple formatters available out there and it would be great to use some pretty generic code that could play well with all of them. I think it's fine that sometimes an additional formatting edit might be required after we insert something into the file, the goal is to minimize the chances of that happening.
Prettier is, of course, vastly popular so we could try to special-case it but I think that this should be done by treating is an optional peer dependency~ whereas right now we always fall back to the local version installed with the CLI if we can't load Prettier relative to cwd
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR to match this on the CLI, but I'm not able to update the vscode extension to do the same.
I kept the prettify method because It's a useful abstraction that can be removed in the future if prettier is entirely removed.
Determine quote style from first found import statement in machine file, then use that for quoting the typegen import.
Closes #351