Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: prettier run on newText #353

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: prettier run on newText #353

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd3dfce
fix: prettier run on newText
dylanarmstrong Jun 17, 2023
c881270
chore: changeset added
dylanarmstrong Jun 17, 2023
f88c38a
feat: use import statement to determine quote style
dylanarmstrong Jun 21, 2023
3db3cfe
chore: changeset
dylanarmstrong Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tasty-bottles-work.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@xstate/cli': patch
---

fix: prettier runs on edited files now
30 changes: 20 additions & 10 deletions apps/cli/src/bin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,26 @@ function getPrettierInstance(cwd: string): typeof import('prettier') {
}
}

// Used to prettify text before writing
const prettify = async (
uri: string,
text: string,
{ cwd }: { cwd: string },
) => {
const prettierInstance = getPrettierInstance(cwd);
return prettierInstance.format(text, {
...(await prettierInstance.resolveConfig(uri)),
parser: 'typescript',
});
};

const writeToTypegenFile = async (
typegenUri: string,
types: TypegenData[],
{ cwd }: { cwd: string },
) => {
const prettierInstance = getPrettierInstance(cwd);
await fs.writeFile(
typegenUri,
// // Prettier v3 returns a promise
await prettierInstance.format(getTypegenOutput(types), {
...(await prettierInstance.resolveConfig(typegenUri)),
parser: 'typescript',
}),
);
const output = await prettify(typegenUri, getTypegenOutput(types), { cwd });
await fs.writeFile(typegenUri, output);
};

// TODO: just use the native one when support for node 12 gets dropped
Expand Down Expand Up @@ -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(
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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.

uri,
processFileEdits(fileContents, edits),
{ cwd },
);
await fs.writeFile(uri, newFile);
}
console.log(`${uri} - success`);
Expand Down