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

Tree-shaking is not working #2497

Closed
nandin-borjigin opened this issue Oct 6, 2018 · 7 comments
Closed

Tree-shaking is not working #2497

nandin-borjigin opened this issue Oct 6, 2018 · 7 comments

Comments

@nandin-borjigin
Copy link

  • Rollup Version: Rollup online REPL
  • Operating System (or Browser): Rollup online REPL
  • Node Version: Rollup online REPL

How Do We Reproduce?

reproductions in REPL

Expected Behavior

Bar-related code should be tree-shaken since it is not used and obviously has no side effects.

  • I'm aware that in some situation, if the "unused" code seems impure(has side effects) to the rollup, rollup would treat it conservatively. But I cannot tell why the code in the reproduction REPL is treated as having a side effect.
  • It's true that foo and bar functions are identical, if anyone concerns.
  • I noticed this problem when I was using rollup-plugin-vue. It transpiles .vue files into independent render functions. Thn importing part of that bundle in another rollup project does not tree-shake the redundant render functions. I traced the problematic code and reproduced it in the REPL.

Actual Behavior

Bar-related code still remains in the bundled output.

@lukastaegert
Copy link
Member

To answer your question: The problem is the property access in

function bar(arg) {
  return arg.options
}

Rollup does not (yet) trace function argument values through function calls. In other words, when checking if calling bar has side-effects, it assumes that arg is an unknown value, e.g. undefined. For an undefined value, accessing a property would be the side-effect of throwing an exception.

There are plans to improve this but it will probably take some time.

So if I understand you correctly, this is a critical piece in tree-shaking Vue.js? This is good to know as this would of course put a little more priority on this.

@nandin-borjigin
Copy link
Author

Thanks for your quick reply!
Technically, it's not tree-shaking Vue.js, it's rather tree-shaking some custom component library based on Vue.js.

rollup-plugin-vue uses vue-component-compiler and it generates a function __vue_normalize__ whose return value is the final export of a .vue file.
image
image

Imagine that there is a vue component library which exports many indepedent basic components such as button, form and etc. We bundles it with rollup and rollup-plugin-vue and the final output is full of copies of this __vue_normalize__ function ( __vue_normalize__, __vue_normalize_$1__ etc).

And in another project, which also uses rollup, I only want to use button from this component library and imports it like import { Button } form 'my-basic-components'. However, the ouput file of the second project would contain all the content of the first one.

@nandin-borjigin
Copy link
Author

I didn't catch the idea why tracking "typeof" matters in this case. No matter how the ternary operator evaluates, the result of the expression would only be relevant to the argument script, and we can statically determine that:

  • the return value of calling __vue_normalize__ has not been used
  • __vue_normalize__ may cause a side effect through its script argument. However, in the unused __vue_normalize__ function call, the script argument passed to it is still a local variable and we know it is not referenced anywhere else.
  • we didn't call __vue_normalize__ with script = undefined

Bingo, tree-shake it

I'm aware that this might be childish analysis, but I think it make sense, right?

@lukastaegert
Copy link
Member

Yes, the typeof was indeed irrelevant, my bad.

vue_normalize may cause a side effect through its script argument. However, in the unused vue_normalize function call, the script argument passed to it is still a local variable and we know it is not referenced anywhere else

Yes, but we still need to track the function call parameter to know script.options can be safely accessed. This is the ultimate goal but will take time.

@nandin-borjigin
Copy link
Author

nandin-borjigin commented Oct 7, 2018

This is the ultimate goal

Happy to hear that! 😄

@shellscape
Copy link
Contributor

Hey folks. This is a saved-form message, but rest assured we mean every word. The Rollup team is attempting to clean up the Issues backlog in the hopes that the active and still-needed, still-relevant issues bubble up to the surface. With that, we're closing issues that have been open for an eon or two, and have gone stale like pirate hard-tack without activity.

We really appreciate the folks have taken the time to open and comment on this issue. Please don't confuse this closure with us not caring or dismissing your issue, feature request, discussion, or report. The issue will still be here, just in a closed state. If the issue pertains to a bug, please re-test for the bug on the latest version of Rollup and if present, please tag @shellscape and request a re-open, and we'll be happy to oblige.

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

3 participants