Skip to content

feat: optimize in checks on namespaces to keep them treeshake-able #6029

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 5 commits into from
Jul 27, 2025
Merged

feat: optimize in checks on namespaces to keep them treeshake-able #6029

merged 5 commits into from
Jul 27, 2025

Conversation

cyyynthia
Copy link
Contributor

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

Description

Currently, using the in operator on a namespace completely disables treeshaking, even if the left hand-side of the expression can be statically resolved. This makes checking for the existence of an export annoying, as demonstrated here.

This PR optimizes these checks to collapse the expression to a boolean, that is also used for DCE which allows getting rid of eventual compat code and what not in libraries.

There is still a slight annoyance with how warnings are emitted, as shown by the added test. Despite the gate Rollup still emits a warning about the unresolved export. This is minor and I felt like it'd require more invasive changes, so I didn't address this here.

@vercel Vercel
Copy link

vercel bot commented Jul 23, 2025 •
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 Jul 27, 2025 6:03am

This was referenced Jul 23, 2025
Merged
Open
TrickyPi
TrickyPi reviewed Jul 24, 2025
View reviewed changes
lukastaegert
lukastaegert approved these changes Jul 27, 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.

Really nice! I also like how this easily leaves room for extension, e.g. to simplify more binary expressions. The use of deoptimization looks correct to me.

@lukastaegert lukastaegert enabled auto-merge July 27, 2025 05:09
lukastaegert
lukastaegert reviewed Jul 27, 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.

Ok, apparently there is something off in the test, can you check it out?

@lukastaegert
Copy link
Member

Ah, the signature of traceExport had changed on master. I pushed a simple fix.

@codecov Codecov
Copy link

codecov bot commented Jul 27, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@fdd48a9). Learn more about missing BASE report.
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/ast/nodes/BinaryExpression.ts 70.58% 4 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             master    #6029   +/-   ##
=========================================
  Coverage          ?   98.75%           
=========================================
  Files             ?      271           
  Lines             ?    10597           
  Branches          ?     2828           
=========================================
  Hits              ?    10465           
  Misses            ?       91           
  Partials          ?       41

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lukastaegert lukastaegert added this pull request to the merge queue Jul 27, 2025
Merged via the queue into rollup:master with commit 9a8614f Jul 27, 2025
41 checks passed
@github-actions GitHub Actions
Copy link

This PR has been released as part of [email protected]. You can test it via npm install rollup.