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.

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

Finish v4 rewrite of Prettier's CLI #154

Open
fabiospampinato opened this issue Jan 27, 2025 · 3 comments
Open

Finish v4 rewrite of Prettier's CLI #154

fabiospampinato opened this issue Jan 27, 2025 · 3 comments

Comments

@fabiospampinato
Copy link

fabiospampinato commented Jan 27, 2025

I have a work-in-progress rewrite of Prettier's CLI here. It should be very usable already, but it's probably not mature enough to be shipped to millions of people yet.

I had written a blog post about it here, and basically this is the performance improvement I had measured against Babel's repo.

We should get this shipped as stable sooner rather than later, but I've been short on free time compared to when I started the rewrite, so I could use some help for pushing this across the finish line and actually ship it to millions of people.

If you are interested in helping out I think 90% of the remaining work is basically just porting existing integration tests from Prettier's repo to the new CLI's repo, to fix some minor differences between the two implementations when it makes sense to do so, or at least document these minor differences when it's dubious whether the behavior of the old CLI is more desirable or not.

How to port an integration test

  1. Prettier's actual integration tests are located here. The goal is essentially to copy these over to the CLI's repo, here.
  2. Some integration tests are snapshot-based, the original snapshots are located here, they should be copied here.
  3. Most tests also target some fixture/sample directories, located either here or here, they should be copied here.

For how to port a test in practice you should read some of the recent commits that port tests here, it's boring but it's the most useful thing you can do to ramp up. For example:

  1. This commit ports over the "check" test suite.
  2. The original test file was here, the ported test file is here.
  3. Originally those tests were targeting the cli/with-shebang and cli/write fixture folders, which were copied here and here respectively. The linked commit doesn't copy them over because they had been copied over already as they were needed for other tests.
  4. The original snapshot file for that test was here, and it was ported over here.

Now, if you want to help out I think we should do it this way:

  1. First of all find a test file here that hasn't been ported over already, I'd suggest to start with small-ish ones. Check this table that lists missing tests.
  2. Try to get a sense of how easy it would be to port it over, or whether it could be ported over at all. Some files test options that got deleted in the rewrite, for example the way caching works is completely different, they don't need to be ported over. Some test files use undocumented debugging options, those are not implemented in the rewrite and it may be painful to port them over.
  3. Once you've found a test that you'd like to help port over check if there is an issue about it here already, if there's a pull request here already, or if somebody mentioned they are working on it here. If nobody is working on it yet do one of those things yourself to signal to others you are handling that test file.
  4. Copy over the eventual snapshot file, and any eventual fixture directory that's needed, and only those needed by the file you are porting over that are missing.
  5. Copy over the actual test file, and most importantly rewrite the implicitly-imported runCli("cli/foo", ... calls in it into the explicitly imported runCli("foo", ... calls. Tests are still executed via Jest, but in a different way, as I couldn't figure out the extremely convoluted way tests were being ran in the old implementation, so I reimplemented that differently, here's the new implementation. Some minor things are not implemented, like the stdoutIsTTY option.
  6. To run all tests in the repo you can just do a npm run test, which will execute the following: cross-env NODE_OPTIONS=--experimental-vm-modules jest --rootDir test/__tests__ --testTimeout 60000.
  7. While you are working on a single test you probably just want to execute a single test instead though, which you can do by running something like this: NODE_OPTIONS=--experimental-vm-modules npx jest --rootDir test/__tests__ FILE_NAME.js. Now, if you are particularly lucky all tests will simply pass, in which case feel free to just make a PR with your changes, which I'll approve. Most probably there will be some issues, at this point we need to do some of the following:
  8. Is the test failing but we are ok with the new behavior/output? It could be that some slightly different string is outputted to the user, or an error code 1 is returned instead of an error code 2 (no error code 2 are returned in the rewrite). If that's the case we should update the shapshot and/or the test to make it match the new behavior, and document this slight difference, to make sure users will be informed about it. Not all slight differences are acceptable though! Some could be important bugs.
  9. Is the test failing and do we think there is something that should be changed in the rewritten CLI? If so we can .skip the test for now and deal with it later, or you can submit a patch for that too if you know what the change should be, or feel free to just ask about what should be done in the open issue or the open PR, some issues are weird.

Basically just porting over the file with some .skipped tests in it is already valuable, all changes should be known and documented in preparation of the changelog for the stable release, and for some failing tests we are fine with the new behavior so we can just tweak the tests, while for others there is actually something to change in the rewritten CLI or its dependencies.

Almost every dependency of the rewritten CLI is something I maintain, so if something should be changed there it's not a problem to submit a patch there directly too, we don't need to wait on anybody else to fix things.

That's about it for porting tests over. The remaining work before a stable release can be made is basically about adding some extra tests for new options, and going over some of the TODOs and FIXMEs inlined in the repo.

Thanks for reading, and especially if you want to help! ❤️

Feel free to ping me anywhere you like if you need some clarifications or if you need to be unblocked on something.

@43081j
Copy link
Collaborator

43081j commented Jan 27, 2025

i think if you pick something up, its worth creating an issue in the cli repo or posting here so people are aware, too (less stepping on toes)

this is great though. if we can work through this together, we can start migrating people to the new CLI for sure

@43081j
Copy link
Collaborator

43081j commented Jan 27, 2025

i've created prettier/prettier-cli#19 to help people pick tests to move across

if you can post in there if you do any, or if you find one we shouldn't migrate, i will update the list

@fabiospampinato
Copy link
Author

fabiospampinato commented Feb 9, 2025

We are using this Discord thread in the e18e server for coordinating on this also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants