Skip to content
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

fix: mark some known globals or their functions as impure #4955

Merged
merged 10 commits into from Apr 30, 2023
Merged

fix: mark some known globals or their functions as impure #4955

merged 10 commits into from Apr 30, 2023

Conversation

TrickyPi
Copy link
Member

@TrickyPi TrickyPi commented Apr 23, 2023 •
edited

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 #4282

Description

Currently I mark Map is impure as a temporary resolution. I noticed that there are some issues related to the tree-shaking to the iterator, this #4282 issue is also similar.

@vercel
Copy link

vercel bot commented Apr 23, 2023 •
edited

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 Apr 29, 2023 0:36am

@codecov
Copy link

codecov bot commented Apr 23, 2023 •
edited

Codecov Report

Merging #4955 (39def64) into master (703e88f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4955   +/-   ##
=======================================
  Coverage   98.99%   98.99%           
=======================================
  Files         222      222           
  Lines        8123     8123           
  Branches     2237     2237           
=======================================
  Hits         8041     8041           
  Misses         28       28           
  Partials       54       54
Impacted Files Coverage Δ
src/ast/nodes/shared/knownGlobals.ts 100.00% <ø> (ø)

@TrickyPi
Copy link
Member Author

TrickyPi commented Apr 23, 2023 •
edited

After implementing #3127, we can further improve the tree-shaking to the Map.

lukastaegert
lukastaegert reviewed Apr 23, 2023
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.

Looks good to me, but the test case should be modified so that it breaks with current Rollup.

test/function/samples/tree-shake-map-constructor/main.js Outdated Show resolved Hide resolved
@dnalborczyk
Copy link
Contributor

wouldn't this apply to Set, WeakMap, WeakSet and other static methods consuming Iterables such as Object.fromEntries as well?

@TrickyPi TrickyPi changed the title fix: mark Map is impure fix: mark some known globals or their functions as impure Apr 25, 2023
@TrickyPi
Copy link
Member Author

Thanks for your review! we should apply this to what you mentioned, I marked more known globals or their methods as impure.

lukastaegert
lukastaegert approved these changes Apr 30, 2023
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.

Thanks a lot!

@lukastaegert lukastaegert added this pull request to the merge queue Apr 30, 2023
Merged via the queue into rollup:master with commit f8977a7 Apr 30, 2023
12 checks passed
@rollup-bot
Copy link
Collaborator

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

@rollup-bot rollup-bot mentioned this pull request Apr 30, 2023
Closed
@abdulsattar abdulsattar mentioned this pull request May 19, 2023
Merged
@TrickyPi TrickyPi deleted the fix/map-impure branch May 23, 2023 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnalborczyk dnalborczyk dnalborczyk left review comments

@lukastaegert lukastaegert lukastaegert approved these changes

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

Successfully merging this pull request may close these issues.

Rollup deletes code with side effects due to incorrect pure annotations
4 participants
@TrickyPi @dnalborczyk @rollup-bot @lukastaegert