Skip to content

Comments

feat: tree-shake deterministic dynamic imports#4952

Merged
lukastaegert merged 26 commits intorollup:masterfrom
antfu:feat/treeshake-dynamic-import
Apr 23, 2023
Merged

feat: tree-shake deterministic dynamic imports#4952
lukastaegert merged 26 commits intorollup:masterfrom
antfu:feat/treeshake-dynamic-import

Conversation

@antfu
Copy link
Contributor

@antfu antfu commented Apr 20, 2023 •
edited
Loading

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

fix #3417
fix #3447

Description

Full proposal in #4951

This PR enables tree-shaking for deterministic dynamic imports. The following are valid cases:

const { foo } = await import('foo')
(await import('foo')).foo
import('bar').then(({ foo }) => {})

Idea credits to:

@vercel
Copy link

vercel bot commented Apr 20, 2023 •
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 Apr 23, 2023 7:23pm

@codecov
Copy link

codecov bot commented Apr 20, 2023 •
edited
Loading

Codecov Report

Merging #4952 (7a0ab4c) into master (5af8799) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4952   +/-   ##
=======================================
  Coverage   98.98%   98.98%           
=======================================
  Files         222      222           
  Lines        8050     8112   +62     
  Branches     2210     2236   +26     
=======================================
+ Hits         7968     8030   +62     
  Misses         28       28           
  Partials       54       54
Impacted Files Coverage Δ
src/Module.ts 100.00% <100.00%> (ø)
src/ast/nodes/ImportExpression.ts 100.00% <100.00%> (ø)
src/ast/variables/NamespaceVariable.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@antfu antfu force-pushed the feat/treeshake-dynamic-import branch from 04cd599 to 913bb4c Compare April 20, 2023 21:32
@antfu antfu force-pushed the feat/treeshake-dynamic-import branch from 056a017 to 9ac9832 Compare April 20, 2023 23:18
@antfu antfu requested a review from lukastaegert April 22, 2023 08:36
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.

Just some final comments and a possible optimization I stumbled upon.

lukastaegert
lukastaegert approved these changes 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.

Perfect, thanks a lot!

@lukastaegert lukastaegert enabled auto-merge April 23, 2023 19:22
@lukastaegert lukastaegert added this pull request to the merge queue Apr 23, 2023
Merged via the queue into rollup:master with commit b79b73c Apr 23, 2023
@antfu antfu deleted the feat/treeshake-dynamic-import branch April 23, 2023 19:41
@rollup-bot
Copy link
Collaborator

rollup-bot commented Apr 23, 2023

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

This was referenced Apr 23, 2023
Closed
Closed
Closed
dasa
dasa reviewed Apr 24, 2023
View reviewed changes
src/ast/nodes/ImportExpression.ts
Comment on lines +61 to +63
* - `const { foo } = await import('bar')`.
* - `(await import('bar')).foo`
* - `import('bar').then(({ foo }) => {})`
Copy link
Contributor

@dasa dasa Apr 24, 2023

Choose a reason for hiding this comment

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

Should this be doc'ed here too? https://rollupjs.org/introduction/#tree-shaking

Copy link
Member

@lukastaegert lukastaegert Apr 24, 2023

Choose a reason for hiding this comment

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

Any PR for the docs is welcome! I guess something similar to what Parcel has does not hurt.

Copy link
Contributor

@dasa dasa Apr 25, 2023

Choose a reason for hiding this comment

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

Is this what you're referring to? https://parceljs.org/features/scope-hoisting/#dynamic-imports

Copy link
Member

@lukastaegert lukastaegert Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

👍
And we might even include a REPL link to see it in action and play around

@keller-mark keller-mark mentioned this pull request May 10, 2023
Merged
3 tasks
@pcfreak30 pcfreak30 mentioned this pull request Jun 24, 2023
Closed
@lukastaegert lukastaegert mentioned this pull request Jun 25, 2023
Merged
9 tasks
@benlind benlind mentioned this pull request Aug 17, 2023
Closed
7 tasks
@sun0day sun0day mentioned this pull request Aug 28, 2023
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

+1 more reviewer

@dasa dasa dasa left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

x⁸ ⚙️ build repl artefacts

Projects

None yet

Milestone

No milestone

4 participants

@antfu @lukastaegert @rollup-bot @dasa