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

Build a single-file cli #735

Closed
wants to merge 1 commit into from
Closed

Build a single-file cli #735

wants to merge 1 commit into from

Conversation

vjeux
Copy link
Contributor

@vjeux vjeux commented Feb 17, 2017

Right now, doing npm install of prettier creates 3500 files, which is a pain to manage and to check in. Using the awesome rollup, we can easily bundle all the dependencies into a single big js file. This way I can just check-in a single file in fb codebase and update it when there's a new version.

Performance-wise, it is currently 50ms slower than using ./bin/prettier. Both babylon and flow are taking roughly 30ms to parse and we only need a single one at a time. We can shave off 20ms by minifying the bundle but it breaks flow parser (this is why we don't have a flow option on the website!)

rollup doesn't support shebangs so I had to split the bin/prettier into two files, a dummy one with the shebang and I put the code inside of src: src/cli.js.

Fixes #733

Right now, doing `npm install` of prettier creates 3500 files, which is a pain to manage and to check in. Using the awesome rollup, we can easily bundle all the dependencies into a single big js file. This way I can just check-in a single file in fb codebase and update it when there's a new version.

Performance-wise, it is currently 50ms slower than using ./bin/prettier. Both babylon and flow are taking roughly 30ms to parse and we only need a single one at a time. We can shave off 20ms by minifying the bundle but it breaks flow parser (this is why we don't have a flow option on the website!)

rollup doesn't support shebangs so I had to split the bin/prettier into two files, a dummy one with the shebang and I put the code inside of src: `src/cli.js`.
@jlongster
Copy link
Member

I'm curious, why would an integration need its own bundle? If it's JS-based, like atom, it already supports npm dependencies. If it's other tools like Emacs/Vim/etc they just rely on an external "prettier" command to be available which you can install with npm install -g prettier or install somewhere else.

That's how all integrations are working now so what is different about your setup that makes npm install a pain?

@jlongster
Copy link
Member

50ms is a decent amount (on top of an already 200-300ms as far as I know).

Another option would be to disable some dependencies. If you are doing integration, you shouldn't need a bunch of the CLI stuff like chaulk, get-stdin, minimist, etc. Can we somehow make those optional?

@vjeux
Copy link
Contributor Author

vjeux commented Feb 18, 2017

Here are all the constraints:

  • We have multiple repositories with JavaScript files. Each repo have a few places where there are bulks of js (Nuclide, React Native, Web, Internal tools...)
  • Yarn and checked in cache is only setup for a few subsets but not all
  • npm install prettier generates 3500 files which puts stress on phabricator and is really hard to review / update.
  • We want to use prettier both inside of the editor and inside of our lint infrastructure.
  • Most of the development is done on a devserver using Nuclide to connect remotely.

I want prettier to be run super fast when the hotkey is triggered so I need two things:

  • Get the prettier version that is installed on the remote repository and run it locally somehow in order to avoid the roundtrip.
  • Maintain prettier process active when running the subsequent formats in order to avoid paying the startup cost everytime.

Because we have to use the same version of prettier for linting and nuclide, we need to have the repo being the source of truth.

Because we have it on multiple repos, there are likely going to be multiple versions of prettier deployed for some period of time.

In order to download the installed version of prettier on the remote repository, having it being a single file is waaayyyy easier.

@jlongster
Copy link
Member

npm install prettier generates 3500 files which puts stress on phabricator and is really hard to review / update.

How do you all you use other modules from npm? Do you always bundle them together? We don't have too many dependencies, and you shouldn't need the CLI, so we could get rid of half of them if we figure out how to make them optional. Or are you still invoking it via the CLI?

If you use other npm modules, does your infrastructure provide a way to do this automatically for any npm module?

In order to download the installed version of prettier on the remote repository, having it being a single file is waaayyyy easier.

That makes sense. So are you planning on installing it by simply curl-ing the bundle file from a tagged version from github? What if you want a different version of prettier? I guess it seems easier to be able to do npm install prettier@0.17.0 and then generate the bundle in your build process; then you can always install it through npm like normal.

@vjeux
Copy link
Contributor Author

vjeux commented Feb 18, 2017

Soooo, our npm story is a giant mess.

  • On React Native we use yarn global cache so we check in a tarball for each dependency and all the commands that need them need to call a small script first that extracts them into the node_modules/ folder.
  • On Nuclide in development mode, developers need to run a custom script that does npm install under the hood. For production, we let atom package manager download those dependencies.
  • On web and many other places, we check in node_modules in source control.

There are two main ways Nuclide deals with remote connections:

  • Execute everything remotely. This way the source of truth is the same as the repo, super easy but you need a roundtrip for every action.
  • Have the same version installed locally. Either by pushing things to all the laptops or embed it inside of Nuclide itself. Nuclide is on a weekly release schedule so it's super long. The infra to push to laptops is a pain to deal with. You need to make sure that you pushed all the possible versions before you can actually use them which is annoying.

the great thing about prettier is that it's written in js so we don't have to worry about binaries not working on different architectures. And, we can bundle it into a single file and download it on the fly.

For downloading from npm, we don't want to for security and stability reasons. We check in things inside of the repo. In practice, I would just take the bundled file from the release and check it in the repo.

One last consideration is that whenever I upgrade the version, I'll need to codemod all the existing file with the new version, so that developers don't see warnings when they edit files that haven't changed but are pretty printed differently across revisions.

Having a single file update means that I can do the codemod and upgrade in one diff. If it was a checked in npm source, the phabricator diff would be all garbage because of all the dependency changes and harder to review.

@jlongster
Copy link
Member

In practice, I would just take the bundled file from the release and check it in the repo

I was wondering if you could generate the bundle outside of prettier, but I have experience with similar messy setups in Firefox. We had to do a lot of similar things and it was annoying.

I'm fine having this in here so it's ready for others to use. Eventually we could also have a version that just has the API and can be used inside browsers too.

Thanks for explaining -- it's good to know the constraints of your setup.

If you rebase and fix the conflicts, we can merge this. We should also look into automating the release process a little better now too, since it's especially important to have this file up-to-date (maybe for now that's as simple as having a npm run release command that does anything needed when tagging a version).

@vjeux
Copy link
Contributor Author

vjeux commented Feb 25, 2017

I've setup a separate project for this: https://github.com/vjeux/prettier-rpc

We'll likely want to integrate it in the official project at some point but it should be easier to iterate on it outside of the main repo for now.

@vjeux vjeux closed this Feb 25, 2017
@vjeux vjeux mentioned this pull request Mar 8, 2017
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants