8000 feat: enhance the treehshaking for unary expression by TrickyPi · Pull Request #5775 · rollup/rollup · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: enhance the treehshaking for unary expression #5775

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 6, 2025
Merged

feat: enhance the treehshaking for unary expression #5775

merged 4 commits into from
Jan 6, 2025

Conversation

TrickyPi
Copy link
Member

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
resolves #5732

Description

If the argument has no side effects and the unary expression can evaluate to a literal value, I think we can use that literal value directly in the finalized code.

@vercel Vercel
Copy link
vercel bot commented Dec 29, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2025 0:24am

@TrickyPi TrickyPi force-pushed the feat/treeshakes-unary branch from ff8b5fa to 8926159 Compare December 29, 2024 04:36
@github-actions GitHub Actions
Copy link
github-actions bot commented Dec 29, 2024 •
edited
Loading

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#feat/treeshakes-unary

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:
https://rollup-glzkgf4l8-rollup-js.vercel.app/repl/?pr=5775

@github-actions GitHub Actions
Copy link
github-actions bot commented Dec 29, 2024 •
edited
Loading

Performance report

  • BUILD: 7291ms, 734 MB
    • initialize: 0ms, 27.2 MB
    • generate module graph: 2736ms, 550 MB
      • generate ast: 1240ms, 543 MB
    • sort and bind modules: 393ms, 593 MB
    • mark included statements: 4167ms, 734 MB
      • treeshaking pass 1: 1368ms, 695 MB
      • treeshaking pass 2: 666ms, 723 MB
      • treeshaking pass 3: 275ms, 723 MB
      • treeshaking pass 4: 255ms, 725 MB
      • treeshaking pass 5: 299ms, 729 MB
      • treeshaking pass 6: 243ms, 730 MB
      • treeshaking pass 7: 226ms, 731 MB
      • treeshaking pass 8: 220ms, 733 MB
      • treeshaking pass 9: 205ms, 730 MB
      • treeshaking pass 10: 206ms, 733 MB
      • treeshaking pass 11: 205ms, 734 MB
  • GENERATE: 692ms, 973 MB
    • initialize render: 0ms, 869 MB
    • generate chunks: 71ms, 879 MB
      • optimize chunks: 0ms, 874 MB
    • render chunks: 606ms, 948 MB
    • transform chunks: 15ms, 973 MB
    • generate bundle: 0ms, 973 MB

@codecov Codecov
Copy link
codecov bot commented Dec 29, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.01%. Comparing base (f5c349e) to head (97e4b83).
Report is 2 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #5775   +/-   ##
=======================================
  Coverage   99.01%   99.01%           
=======================================
  Files         259      259           
  Lines        8090     8114   +24     
  Branches     1363     1370    +7     
=======================================
+ Hits         8010     8034   +24     
  Misses         53       53           
  Partials       27       27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TrickyPi
TrickyPi commented Dec 30, 2024
View reviewed changes
lukastaegert
lukastaegert reviewed Jan 4, 2025
View reviewed changes
Copy link
Member
@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a really cool idea and should usually provide a nice benefit. It can even help to tree-shake code, which is especially cool. I have some comments to look at, but in general I like the feature!

@TrickyPi
Copy link
Member Author
TrickyPi commented Jan 5, 2025

I've been struggling with this for a long time, and I feel that the smallest representation can be delegated to specialized compression tools like Terser. Therefore, I feel that we just need to focus on improving tree-shaking and making the resulting code as straightforward and easy to understand as possible. Because of this, I've retained true, false, undefined, and -Infinity. However, when a UnaryExpression evaluates to a number, I handle it specifically to prevent cases where scientific notation numbers are converted into very long numbers. Furthermore, I feel we don't really need to consider shortening literals in the Literal nodes. Lukas, what do you think about this? I'd love to hear your thoughts.

@TrickyPi TrickyPi force-pushed the feat/treeshakes-unary branch from 97eb7e6 to 97e4b83 Compare January 5, 2025 12:22
@lukastaegert
Copy link
Member
lukastaegert commented Jan 6, 2025 •
edited
Loading

I've been struggling with this for a long time, and I feel that the smallest representation can be delegated to specialized compression tools like Terser. Therefore, I feel that we just need to focus on improving tree-shaking and making the resulting code as straightforward and easy to understand as possible. Because of this, I've retained true, false, undefined, and -Infinity. However, when a UnaryExpression evaluates to a number, I handle it specifically to prevent cases where scientific notation numbers are converted into very long numbers. Furthermore, I feel we don't really need to consider shortening literals in the Literal nodes. Lukas, what do you think about this? I'd love to hear your thoughts

That sounds good to me, and I see your point with numbers. And thank you for taking the time to think this through.

@lukastaegert lukastaegert enabled auto-merge January 6, 2025 06:05
@lukastaegert lukastaegert added this pull request to the merge queue Jan 6, 2025
Merged via the queue into master with commit d3e2bf7 Jan 6, 2025
42 checks passed
@lukastaegert lukastaegert deleted the feat/treeshakes-unary branch January 6, 2025 06:15
@github-actions GitHub Actions
Copy link
github-actions bot commented Jan 6, 2025

This PR has been released as part of rollup@4.30.0. You can test it via npm install rollup.

@github-actions github-actions bot mentioned this pull request Jan 6, 2025
Closed
@tarik02 tarik02 mentioned this pull request Jan 6, 2025
Closed
@TrickyPi TrickyPi mentioned this pull request Jan 6, 2025
Merged
9 tasks
@typhonrt
Copy link

Hi @TrickyPi.

In updating my projects from a previous version of Rollup I noticed that this PR converts void 0 to undefined in build output. I personally out of historical / creature of habit reasons have a lot of code that uses void 0. I'm well aware that using void 0 vs undefined doesn't have the same ancient use case that formed my habit from years past. It's just a preference now preferring slightly more compact source code / readability reasons. So, I'd like to understand exactly why the void 0 -> undefined transformation is useful in the context of Rollup. IE I don't like that quite a bit of project / framework code 8000 in source form has void 0, but the build artifacts have undefined instead. It definitely is the 4.30.0 release and this PR that changed this behavior.

Thanks for the insight and your work on Rollup.

@TrickyPi
Copy link
Member Author
TrickyPi commented Feb 2, 2025

It's just a preference now preferring slightly more compact source code / readability reasons. So, I'd like to understand exactly why the void 0 -> undefined transformation is useful in the context of Rollup.

It's just a final output representation. While choosing between void 0 and undefined to represent the result of a unary expression that might be undefined (although currently, only void operations produce undefined), I ended up prioritizing readability.

@typhonrt
Copy link
typhonrt commented Feb 4, 2025

That's the thing. Readability is subjective. In my larger / complex project with many build assets with some that are intermediary / checked in to repos I immediately noticed this change. I'm at the end release candidate stage with many pre-releases prior just before a final release for this effort and updated Rollup from 4.29.x to latest for the last RC and this was a concern that needed to be investigated when every build artifact had unexpected changes.

From a readability point of view for those that use void 0 instead of undefined the annoyance is that for end devs looking at the build assets in IDEs they may be slightly confused when turning to the actual source repos. If there was a functional reason for the transformation that is understandable. A transformation like this might better be handled as opt-in otherwise, but I also grant that I'm likely in a small minority on the readability preference of void 0 over undefined.

@TrickyPi TrickyPi mentioned this pull request Feb 7, 2025
Merged
9 tasks
@TrickyPi
Copy link
Member Author
TrickyPi commented Feb 8, 2025 •
edited
Loading

@typhonrt Changed in this PR #5838

@typhonrt
Copy link

Awesome.. Thanks for the consideration!

@OmkarJ13 OmkarJ13 mentioned this pull request May 29, 2025
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukastaegert lukastaegert lukastaegert approved these changes

Assignees
5009 No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Condense !!true to true
3 participants
@TrickyPi @lukastaegert @typhonrt
0