-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: improve tree-shaking for dynamic import #5420
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
c8a0fc6
to
2f541ee
Compare
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install rollup/rollup#feat/tree-shaking-5410 Notice: Ensure you have installed the latest stable Rust toolchain. If you haven't installed it yet, please see https://www.rust-lang.org/tools/install to learn how to download Rustup and install Rust. or load it into the REPL: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5420 +/- ##
==========================================
- Coverage 98.81% 98.80% -0.02%
==========================================
Files 238 238
Lines 9544 9617 +73
Branches 2439 2463 +24
==========================================
+ Hits 9431 9502 +71
- Misses 48 49 +1
- Partials 65 66 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for picking this up! This already works quite well in many cases, but I found a few issues that should be resolved before we merge this. See this example
Basically at the moment if you access some properties of the namespace by name, it will ignore all indirect property accesses, which include
- via unknown/statically not resolvable computed properties
- via destructuring
- via passing the namespace to a function
- via assigning it to another variable
- via accessing it through a sequence, conditional or logical expression
Not sure if this list is complete. In all of these cases, one should probably include all exports. On the other hand if we can handle all of those cases, there is no reason to include any exports by default if the namespace is not accessed at all.
The question is how to do this elegantly. One way of addressing this could be to try to handle all edge cases. Another could be to try to implement the bigger feature of object property tree-shaking.
To do that, one could basically replace the current include
method with an includePath(path, context, includeChildrenRecursively, options)
method. By default, this would still include the whole object of which we access a single path here
rollup/src/ast/nodes/shared/Expression.ts
Line 76 in 10bdaa3
return true; |
However, we can override the method in some strategic locations. I.e. if we include a path
['foo']
of a member expression bar.baz
, instead it includes the path ['baz', 'foo']
of bar
. We can also shorten paths to prevent some circularity issues here as it is never a problem to include "too much", especially if some path segments are unknown.
That being said, I would really like to encourage you to do this. This could become a feature I literally wanted to implement for years. Basically what you need to do is
In another step, we could add special handling to One question is what to do with destructuring. At the moment, destructuring will not be able to track paths, which is something you also see when tracking values like here https://rollup-ciqghdigc-rollup-js.vercel.app/repl/?pr=5420&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMmNvbnN0JTIwb2JqJTIwJTNEJTIwJTdCZm9vJTNBJTIwdHJ1ZSUyQyUyMGJhciUzQSUyMGZhbHNlJTdEJTNCJTVDbmlmJTIwKG9iai5iYXIpJTIwY29uc29sZS5sb2coJ3JlbW92ZWQnKSUzQiU1Q24lNUNuY29uc3QlMjAlN0JiYXIlN0QlMjAlM0QlMjBvYmolM0IlNUNuaWYlMjAoYmFyKSUyMGNvbnNvbGUubG9nKCdub3QlMjByZW1vdmVkJTIwZXZlbiUyMHRob3VnaCUyMGl0JTIwc2hvdWxkJTIwYmUnKSUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTJDJTIybmFtZSUyMiUzQSUyMm1haW4uanMlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyb3V0cHV0JTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlN0QlMkMlMjJ0cmVlc2hha2UlMjIlM0F0cnVlJTdEJTdE But again, this could be a separate improvement. |
Wow, thank you so much for your professional and patient guidance! I will thoroughly digest this knowledge (object property tree-shaking) and then work on implementing it. |
My pleasure! You are doing great work and it turns out sharing knowledge and ideas with you is always a good investment 😉 |
2f541ee
to
eda5b5a
Compare
eda5b5a
to
080b10f
Compare
40f1b43
to
c6c2366
Compare
c6c2366
to
adf93ab
Compare
65a67f3
to
4f938e3
Compare
e4820e5
to
903548e
Compare
869a45c
to
6aed230
Compare
3211811
to
98f4026
Compare
15163e7
to
05059a5
Compare
Hi, Lukas. I have refactored this implementation. If you have some time, could you please review it again? |
0e9ddc0
to
afec37e
Compare
afec37e
to
6619c7e
Compare
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
related #5410
Description
Work in progress