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

[discussion] Tree shake-ability #138

Closed
Andarist opened this issue Oct 1, 2017 · 19 comments
Closed

[discussion] Tree shake-ability #138

Andarist opened this issue Oct 1, 2017 · 19 comments

Comments

@Andarist
Copy link
Contributor

Andarist commented Oct 1, 2017

At the current state the library, although highly modular (👏), cannot be effectively tree-shaken (well, dead code eliminated to be more exact), because of the heavy curry usage. It is like that because bundlers/minifiers cannot know for sure that the call to curry is pure and wont cause any side-effects - they cannot be sure that it is safe to remove that call, even if its result stays unused.

UglifyJS has introduced possibility to mark calls as pure with comment annotation - #__PURE__. This could be leveraged in the library.

While #__PURE__ comments are useful today, it's quite ugly to put them into the source code and also it's error-prone, it's easy to forget to put them everywhere during development. I've created a babel plugin which helps with that and automates this task, but at the moment you are using buble and not babel for es2015+ transpilation and Im not sure if you are open to a change and if you want your code to get annotated (in any way, doesnt have to be with a plugin I've created).

So in general - what do you think about this? Would you like me to send a PR with buble -> babel + the plugin?

More reading about the problem - here

@TrySound
Copy link
Contributor

TrySound commented Oct 1, 2017

Also webpack4 will look at side-effects in package.json.
https://medium.com/webpack/road-to-webpack-4-week-20-21-1641d03ce06e#dc50
So it's not necessary to annotate code.

@Andarist
Copy link
Contributor Author

Andarist commented Oct 2, 2017

I'm aware of the side-effects entry for the package.json. It's not the only bundler though and the future of side-effects in others is uncertain at this point in time - also for considerable time many teams won't be able to migrate to the new webpack's version right after the release.

@briancavalier
Copy link
Member

Hi @Andarist, thanks for raising this issue!

I'm in favor of anything that helps, as long as we feel the concrete benefit to users outweighs any additional maintenance costs or complexity. From that perspective, it seems like side-effects: false is a no-brainer once it's released.

For the PURE annotations, buble satisfies our needs, and we are close to 1.0, so I'd rather not make a major build system change right now. I'm open to revisiting that after 1.0, though.

Buble does tend to generate leaner code than babel in the first place. I wonder if the tradeoff of adding babel's overhead, but with improved tree-shaking turns out to yield better results for users on average. Do you have any intuition about that?

Would it be possible to have a rollup version of the plugin? Or is there some way to use the existing plugin with rollup, without fully committing to babel right now?

@Andarist
Copy link
Contributor Author

Andarist commented Oct 2, 2017

Buble does tend to generate leaner code than babel in the first place. I wonder if the tradeoff of adding babel's overhead, but with improved tree-shaking turns out to yield better results for users on average. Do you have any intuition about that?

That's really hard to measure, for sure PURE helps - although it is kinda a hack. According to projects using those it can help quite a lot, but also in kinda a specific situations - angular projects are heavily class-based (which ends to be IIFEs when transpiled). Also it helped a lot in ramda's case, which is heavily curry-based - just like the mostjs.

Truth to be told side-effects did quite the same for ramda when webpack 4 (I've tested it with latest master), but that is (for now) webpack-specific feature.

I'm closely looking into those matters lately and it doesnt seem to be 1 perfect solution. We have also started adding #__PURE__ comments in babel's output (i.e. for transpiled classes, tagged template literals and soon to be more). I know that emotion is gonna annotate some of its output too. And there are probably even more examples in the wild I've not seen.

Buble does tend to generate leaner code than babel in the first place.

Agreed, but I think babel's loose mode doesn't add too much overhead over buble's output. Would have to be compared though. It is also not impossible (and even rather easy) to write custom babel transforms for cases when one would want to have even looser output (i.e. similar to buble's).

Would it be possible to have a rollup version of the plugin? Or is there some way to use the existing plugin with rollup, without fully committing to babel right now?

That was my first intuition, but it seems that buble doesn't support plugins, so the only way to add this without commiting to babel would be to create some rollup plugin - aint sure how much work would that be. And ofc there is always an option to use babel only for this single transform - then it should be a matter of adding the babel plugin to rollup's config.

@TrySound
Copy link
Contributor

TrySound commented Oct 2, 2017

The problem is that Babel breaks code style because of code generator. Buble saves it in most cases by nature. I can make plugin. Is there a link about pure annotaton?

@davidchase
Copy link
Member

davidchase commented Oct 2, 2017

@TrySound that would be interesting to see theres a mention of it here from the link the OP shared: https://iamakulov.com/notes/polished-webpack/#solution and some more links and discussions here babel/babel#5632

@TrySound
Copy link
Contributor

TrySound commented Oct 2, 2017

@Andarist
Copy link
Contributor Author

Andarist commented Oct 2, 2017

Yeah, I've created it recently :P It's purpose is different though. It's just a helper which annotates node or path (+ checking if its already annotated - to prevent duplicated annotations). There is no decision involved in the process. (also it's only available since like recent babel7 beta releases)

My annotate-pure-calls plugin visits each CallExpression and NewExpression and check if their results are used in assignment contexts and if they are at the top level (for regular cases this means a program's level) - if both conditions are met, the plugin assumes they are pure and annotates them. You can check the Readme and the tests to get better overview.

@briancavalier
Copy link
Member

use babel only for this single transform

I created a quick proof of concept over in #140 which does that. Thoughts?

@Andarist
Copy link
Contributor Author

Andarist commented Oct 3, 2017

Ok for me, but you guys have mentioned that you care about output formatting (which can get screwed up by babel generator). So you would have to compare pre and post outputs in regards of formatting.

@briancavalier
Copy link
Member

@TrySound Can you explain more what you mean by "Babel breaks code style because of code generator"?

@TrySound
Copy link
Contributor

TrySound commented Oct 3, 2017

Code looks differently. Buble replaces parts of code traversing ast, when babel generates string from ast. It's makes buble output nicer, but makes hard to process all language features and combinations, also introduces a few unexpected bugs which are hard to maintain. Babel is safer, community driven and covers all (and even more) language features. Talking as buble collaborator in the past :)

@briancavalier
Copy link
Member

It seems like you're saying there are 2 potential risks:

  1. buble and babel may conflict with each other somehow and break the most/core build, and
  2. using buble and babel together, in the general case, yields JavaScript that is possibly more "complex" and/or harder for additional tools to process safely.

Am I understanding correctly?

While I do cringe at the added complexity of having both buble and babel in the codebase, we'd be using only a single, focused, babel transform, whose only effect is to insert comments in certain spots. So, we're going from 100% buble output to 99.9% buble output plus pure annotation comments.

That seems reasonably safe, but since I've never done it before, I can't speak to it in practice.

How risky is it in your view?

@TrySound
Copy link
Contributor

TrySound commented Oct 3, 2017

I meant that using buble and babel together is overtooling and you won't get most of benefits of buble with babel after it and buble is not quite safe tool.

@briancavalier
Copy link
Member

Are you saying that babel changes the built output (and not just comments), possibly increasing the size and/or complexity of the built JS, even if we're only using the pure annotation comment plugin?

If so, then yeah, that seems like a very valid concern.

@Andarist
Copy link
Contributor Author

Andarist commented Oct 3, 2017

I think the concern is different. It is purely about the fact that babel uses such approach:
transform -> traverse -> generate

which in other words is:
source -> ast -> source

I think buble + babel can work together, although as mentioned it will prevent (in any configuration used) buble to preserve code style of the input, because it will be parsed with babel too (no matter in which order) which can alter the code style, because it uses its own "style" when generating the output.

Babel won't apply any other transforms than requested, running it without any plugins/presets would result in the very same code (maybe just a little bit reformatted).

@briancavalier
Copy link
Member

See this comment. I'm sure I'm missing something. Perhaps someone who knows these tooling intricacies better than I do would be willing to put together a proof of concept that shows how to make all the things in my comment work. I think that'd help us to evaluate the best way to move forward.

FWIW, I feel ok about releasing 1.0 without pure annotations, while continuing to discuss this.

@Frikki
Copy link
Member

Frikki commented Dec 7, 2017

Now that PR #143 is merged, should this issue still be kept open?

@briancavalier
Copy link
Member

Closed by #143

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

No branches or pull requests

6 participants