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

Unexpected code after tree-shake #6264

Open
Birowsky opened this Issue Jan 8, 2018 · 16 comments

Comments

Projects
None yet
7 participants
@Birowsky
Copy link

Birowsky commented Jan 8, 2018

Do you want to request a feature or report a bug?
Either a bug or unclear docs.

What is the current behavior?
Given entry is the entry module:
screen shot 2018-01-08 at 17 19 34

It just uses function1 from external1 module.

I expected that external1.function2 and the whole external2 module would be eliminated.

However, my output includes external2.function1. Just.. why?

Here's a repo of the whole thing.

@EugeneHlushko

This comment has been minimized.

Copy link
Member

EugeneHlushko commented Jan 14, 2018

While this might not be what you want, but i suggest trying webpack 4 which does not include that external2 thing.

@Birowsky

This comment has been minimized.

Copy link
Author

Birowsky commented Jan 16, 2018

@EugeneHlushko tried compiling with:

    "webpack": "4.0.0-alpha.4",
    "webpack-cli": "2.0.4"

Got:

screen shot 2018-01-16 at 10 41 43

@sokra

This comment has been minimized.

Copy link
Member

sokra commented Jan 16, 2018

This doesn't work yet. We don't do this kind of scope analysis yet, but this is prepared in some places and could be added.

@chengluyu

This comment has been minimized.

Copy link

chengluyu commented Mar 23, 2018

By using ModuleConcatenationPlugin, webpack does eliminate external2 stuff correctly.

Here's output from webpack 4 with ModuleConcatenationPlugin and UglifyJSPlugin: gist link.

@chengluyu

This comment has been minimized.

Copy link

chengluyu commented Mar 23, 2018

@Birowsky You really should try out webpack 4 and ModuleConcatenationPlugin

And don't forget to upgrade ts-loader to 4.x to avoid the error Cannot read property 'ts' of undefined.

Or you can just try out my fork of your repo.

@chengluyu

This comment has been minimized.

Copy link

chengluyu commented Mar 23, 2018

@sokra I think although there's no deep scope analysis strategy in webpack, by using ModuleConcaatenationPlugin and minifiers that supports dead code removal (e.g. UglifyJSPlugin), the same result can be achieved.

@Birowsky

This comment has been minimized.

Copy link
Author

Birowsky commented Mar 24, 2018

@chengluyu hey mate, thanx for figuring it out. However, I don't see you using the ModuleConcatenationPlugin in your fork.

@chengluyu

This comment has been minimized.

Copy link

chengluyu commented Mar 24, 2018

@Birowsky I forgot to push the commit. Updated here.

@Birowsky

This comment has been minimized.

Copy link
Author

Birowsky commented Apr 2, 2018

@chengluyu finally got around to trying this. What I found was that by applying all your changes except ModuleConcatenationPlugin, things still work as I wanted them. Which is nice. But I still wonder, how exactly did you expect the ModuleConcatenationPlugin to help us with this case? I admit I don't quite understand what that plugin does, so if you can dumb it down for me, great, if not, still great :}

@Birowsky Birowsky closed this Apr 2, 2018

@chengluyu

This comment has been minimized.

Copy link

chengluyu commented Apr 3, 2018

@Birowsky The ModuleConcatenationPlugin should make imported modules inline. But it didn't, that's a known limitation.

@tomkel

This comment has been minimized.

Copy link

tomkel commented Apr 3, 2018

Well, this was on the roadmap for 4.x. But now that it's closed..

@Birowsky

This comment has been minimized.

Copy link
Author

Birowsky commented Apr 3, 2018

@tomkel I can reopen it. Is there a reason tho?

@tomkel

This comment has been minimized.

Copy link

tomkel commented Apr 3, 2018

I think this bug report captured the scope analysis you were looking for, for not including code that will not be used. Yes, it can be removed via dead code elimination (#PURE etc), but we can get even smaller bundles ala rollup by implementing strict live code inclusion.

@Birowsky Birowsky reopened this Apr 3, 2018

@vincentdchan

This comment has been minimized.

Copy link

vincentdchan commented May 17, 2018

Hi, everyone. I am implementing this feature. Here's my repo:

https://github.com/vincentdchan/webpack-deep-scope-analysis-plugin

And demo, you can experience the analysis:

https://vincentdchan.github.io/webpack-deep-scope-demo/

If things go well, we can use it very soon. It will come faster if anyone can add more tests or edge cases to my repo. Thank you!

@TheLarkInn

This comment has been minimized.

Copy link
Member

TheLarkInn commented May 19, 2018

@Birowsky have you had a chance to look into the work @vincentdchan has been providing in this case? Hoping to get some feedback for him as this may directly assist with your issue.

@Birowsky

This comment has been minimized.

Copy link
Author

Birowsky commented May 21, 2018

Sorry, I won't be able to have a look into this for the next couple of weeks.

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