Skip to content

Check if left side is included when checking if assigning to an assignment has side effects #6183

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
lukastaegert merged 3 commits into from
Nov 19, 2025
Merged

Check if left side is included when checking if assigning to an assignment has side effects #6183

lukastaegert merged 3 commits into from
Nov 19, 2025
+26 −25

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Nov 16, 2025

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:

Description

The issue is that when assigning to an assignment, we only check if the assignment itself has side effects but not if the assignment target is included.

@vercel
Copy link

vercel bot commented Nov 16, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rollup Ready Ready Preview Comment Nov 16, 2025 2:48pm

@lukastaegert
Shorten copilot instructions
cb4bed9
@lukastaegert
Add failing test
2fa5553
@lukastaegert
Also check left side when assigning to an assignment
2c11742 
Usually, the "included" check in variables is enough to detect if an assignment
has side effects. When assigning to an assignment, however, it is possible to
work around this check and we need to check manually.
@lukastaegert lukastaegert force-pushed the fix-define-property-issue branch from 7a8a2a4 to 2c11742 Compare November 16, 2025 14:47
@lukastaegert lukastaegert mentioned this pull request Nov 16, 2025
Closed
@github-actions
Copy link

github-actions bot commented Nov 16, 2025 •
edited
Loading

Thank you for your contribution! ❤️

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

npm install rollup/rollup#fix-define-property-issue

Notice: Ensure you have installed the latest nightly 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-f6lj0wug1-rollup-js.vercel.app/repl/?pr=6183

@github-actions
Copy link

github-actions bot commented Nov 16, 2025

Performance report

  • BUILD: 6589ms, 819 MB
    • initialize: 0ms, 24.6 MB (+7%)
    • generate module graph: 2348ms, 627 MB
      • generate ast: 1257ms, 618 MB
    • sort and bind modules: 357ms, 683 MB
    • mark included statements: 3859ms, 819 MB
      • treeshaking pass 1: 2300ms, 817 MB
      • treeshaking pass 2: 450ms, 838 MB
      • treeshaking pass 3: 376ms, 820 MB
      • treeshaking pass 4: 361ms, 843 MB
      • treeshaking pass 5: 358ms, 819 MB
  • GENERATE: 631ms, 922 MB
    • initialize render: 0ms, 819 MB
    • generate chunks: 50ms, 841 MB
      • optimize chunks: 0ms, 835 MB
    • render chunks: 565ms, 895 MB
    • transform chunks: 17ms, 921 MB
    • generate bundle: 0ms, 922 MB

@codecov
Copy link

codecov bot commented Nov 16, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.74%. Comparing base (4b4581d) to head (2c11742).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #6183   +/-   ##
=======================================
  Coverage   98.74%   98.74%           
=======================================
  Files         271      271           
  Lines       10639    10640    +1     
  Branches     2847     2848    +1     
=======================================
+ Hits        10505    10506    +1     
  Misses         91       91           
  Partials       43       43

☔ 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 merged commit fb21d56 into master Nov 19, 2025
45 checks passed
@lukastaegert lukastaegert deleted the fix-define-property-issue branch November 19, 2025 05:25
@github-actions
Copy link

github-actions bot commented Nov 19, 2025

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

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tree-shaking removes Object.defineProperty side-effect when using chained assignment

2 participants

@lukastaegert