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

Split bundle before shipping to npm #1640

Closed
vjeux opened this issue May 20, 2017 · 14 comments
Closed

Split bundle before shipping to npm #1640

vjeux opened this issue May 20, 2017 · 14 comments
Labels
help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Milestone

Comments

@vjeux
Copy link
Contributor

vjeux commented May 20, 2017

We're going to support a bunch of parsers that come along with heavy dependencies: babylon, flow, typescript, postcss, and maybe more in the future (graphql? html?)

We can't just use the npm dependency system otherwise it's going to take forever to install prettier and bloat your computer.

What we should do instead is to bundle each parser on its own file that we ship. This way we can have prettier be 0 dependencies and just ship a few files: bin, lib and one for each parser.

@vjeux vjeux added the help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! label May 20, 2017
@CompuIves
Copy link

I started building something similar when I was trying to solve #986. I'll try to finish it tomorrow ☺️.

@mohitgarg
Copy link

That sounds awesome. I would like to help on this issue but I only just a beginner in code splitting if you can help me along the way then I might be able to do this!

@evenstensberg
Copy link

Prefer Gulp + Rollup for this 👍

@LukeSheard
Copy link

Maybe this is a job for lerna? Then ship prettier-clias the core and then prettier-XX language wise?

@CompuIves
Copy link

I think shipping separate packages for all parsers doesn't fit very well here. Mostly because we want to let the user use prettier without doing any prior configuration. For example, if users want to use prettier on typescript they will first need to install prettier-typescript, even though they probably have typescript already installed as a dependency.

I think the best approach would be to let the parser try to require for example typescript if that's specified as the parser, and throw an error with a helpful message if the dependency could not be found.

But maybe I'm missing some context here, would love to get feedback on this.

@vjeux
Copy link
Contributor Author

vjeux commented May 21, 2017

@CompuIves I want prettier on npm to look like:

- bin/prettier
- index.js
- babylon-parser.js
- flow-parser.js
- typescript-parser.js
- postcss-parser.js
- graphql-parser.js

where bin/prettier requires index.js and index.js requires all the xxx-parser.js when needed.

Note that typescript-parser.js is just the pieces of typescript that are needed for prettier to parse typescript, it shouldn't be the entire typescript source code.

Does it make sense?

I think the best approach would be to let the parser try to require for example typescript if that's specified as the parser, and throw an error with a helpful message if the dependency could not be found.

I don't want that. Prettier should only use the version of typescript it is compiled against. Otherwise the AST may change in the process and prettier would not behave correctly.

@CompuIves
Copy link

Ah, that makes sense. Thanks for the clarification!

@joewood
Copy link

joewood commented May 21, 2017

Is that approach going to cause issues when a new feature is added to the AST and prettier's output is based on the old AST? For example, if the TS compile is embedded into Prettier and it's one language feature version behind, then will Prettier's output error out on the new syntax? Or worse erase the code it doesn't understand?

So, if crashing out is OK, then it might as well just make an absolute version binding to the TS compiler rather than bundling (things will break if there's a mismatch if you bundle or if you don't). I can't see how bundling helps if the developer is using a new AST feature. Or am I misunderstanding the issue?

@vjeux
Copy link
Contributor Author

vjeux commented May 21, 2017

The goal of outputting one single file is that we don't have to have prettier depending on huge projects. If you use prettier for flow, you don't want to start pulling all of typescript. By making a single js file for just the typescript parser, it should matter less.

For different versions, all the parsers parse all the old features as well as the new, so being on the latest version in prettier is fine, even if you are using an older in your project.

Even though the code that parser parse is the same, there are often incompatibilities and breaking changes in the ast representation. I have no desire of making prettier support arbitrary versions of those ast.

If there's a new feature introduced by a language, then we need to upgrade prettier to that latest version, otherwise we won't know how to print it.

@joewood
Copy link

joewood commented May 21, 2017

Yeah, agreed download size is an issue. But couldn't the same be accomplished with a peer dependency? If the versions need to match anyway, and if they don't you just fail. Do the dependencies have to be bundled at all?

@vjeux
Copy link
Contributor Author

vjeux commented May 21, 2017

peerDependencies are annoying to manage, if you want to use prettier for css, you shouldn't have to care that you need postcss at version x, postcss-values-parser at version y and postcss-media-query-parser at version z.

It's also a pain for all the editor integrations as they need to use the exact incantation of all those dependencies.

I'd rather put the complexity on prettier itself, so it has 0 npm dependencies and just ships 6 files. This way it's painless to use and no one is going to have second thoughts about adding a ton of dependencies when they use it.

@joewood
Copy link

joewood commented May 21, 2017

Yeah, agreed. I guess with some good tree shaking the bundled files shouldn't be too big.

@CompuIves
Copy link

I've been fighting for a while with this, decided to share the progress here: #1667.

@vjeux vjeux mentioned this issue May 25, 2017
15 tasks
@vjeux
Copy link
Contributor Author

vjeux commented Jul 6, 2017

Done

@vjeux vjeux closed this as completed Jul 6, 2017
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

No branches or pull requests

6 participants