Skip to content

fix: multi declarator tree shake#1564

Merged
IWANABETHATGUY merged 2 commits into
mainfrom
fix/multi-declaration-tree-shake
Jul 10, 2024
Merged

fix: multi declarator tree shake#1564
IWANABETHATGUY merged 2 commits into
mainfrom
fix/multi-declaration-tree-shake

Conversation

@IWANABETHATGUY

@IWANABETHATGUY IWANABETHATGUY commented Jul 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Description

for case:

// index.js
import {a,} from './share'
console.log(a)
// share.js
export const a = 1000, b = 1000;

Before

// share.js
const a = 100, b = 1000;
// main1.js
console.log(a);

After

// share.js
const a = 100;
// main1.js
console.log(a);

@netlify

netlify Bot commented Jul 9, 2024 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit c435721
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/668dfa8654308b000793b4dd

@github-actions

github-actions Bot commented Jul 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Benchmarks Rust

group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.04     52.1±1.54ms        ? ?/sec    1.00     50.1±0.73ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.02     61.8±1.68ms        ? ?/sec    1.00     60.7±1.05ms        ? ?/sec
bundle/bundle@rome-ts                                        1.00     96.5±1.16ms        ? ?/sec    1.00     96.8±1.99ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                              1.01    116.3±1.57ms        ? ?/sec    1.00    115.6±1.47ms        ? ?/sec
bundle/bundle@threejs                                        1.01     29.2±1.11ms        ? ?/sec    1.00     28.9±1.05ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.00     38.4±0.69ms        ? ?/sec    1.01     38.7±0.77ms        ? ?/sec
bundle/bundle@threejs10x                                     1.01   315.3±14.16ms        ? ?/sec    1.00    313.2±4.53ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.02    402.3±7.89ms        ? ?/sec    1.00    394.2±8.00ms        ? ?/sec
scan/scan@rome-ts                                            1.01     77.1±1.23ms        ? ?/sec    1.00     76.6±0.70ms        ? ?/sec
scan/scan@rome-ts-sourcemap                                  1.00     77.9±1.44ms        ? ?/sec    1.00     78.0±1.35ms        ? ?/sec
scan/scan@threejs                                            1.00     21.1±0.38ms        ? ?/sec    1.00     21.1±0.59ms        ? ?/sec
scan/scan@threejs-sourcemap                                  1.01     21.2±0.41ms        ? ?/sec    1.00     21.0±0.29ms        ? ?/sec
scan/scan@threejs10x                                         1.02    213.3±3.90ms        ? ?/sec    1.00    209.3±1.88ms        ? ?/sec
scan/scan@threejs10x-sourcemap                               1.02    213.2±3.86ms        ? ?/sec    1.00    208.1±2.25ms        ? ?/sec

@IWANABETHATGUY IWANABETHATGUY changed the title fix: multi declaration tree shake fix: multi declarator tree shake Jul 10, 2024
@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Jul 10, 2024
Closed
IWANABETHATGUY
IWANABETHATGUY commented Jul 10, 2024
View reviewed changes
Comment thread crates/rolldown/tests/fixtures/tree_shaking/multi-declarator/artifacts.snap

//#region foo.js
const foo = 'foo';
const [c, d] = [1, 2];

@IWANABETHATGUY IWANABETHATGUY Jul 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Both c and d is kept, align with esbuild,
Maybe we could support more advanced tree shaking in the future.

@IWANABETHATGUY IWANABETHATGUY Jul 10, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

https://esbuild.github.io/try/#YgAwLjIwLjIALS1idW5kbGUgLS1taW5pZnk9ZmFsc2UgLS1mb3JtYXQ9ZXNtAGUAZW50cnkuanMACmltcG9ydCB7IGZvbywgYyAgfSBmcm9tICcuL2ZvbycKCgphc3NlcnQuZXF1YWwoZm9vLCAnZm9vJykKYXNzZXJ0LmVxdWFsKGMsIDEpCgAAZm9vLmpzAGV4cG9ydCBjb25zdCBmb28gPSAnZm9vJywgYmFyID0gJ2JhcicKZXhwb3J0IGNvbnN0IFtjLCBkXSA9IFsxLCAyXQo

@IWANABETHATGUY
IWANABETHATGUY force-pushed the fix/multi-declaration-tree-shake branch from a033621 to 60d2afd Compare July 10, 2024 03:04
IWANABETHATGUY
IWANABETHATGUY commented Jul 10, 2024
View reviewed changes
Comment thread packages/rolldown/tests/fixtures/sourcemap/inner/_config.ts
expect(map.file).toMatchInlineSnapshot(`"main.js"`)
expect(map.mappings).toMatchInlineSnapshot(
`";;AAAO,MAAM,MAAM;;;;ACEnB,QAAQ,IAAI,IAAI"`,
`";;AAAA,MAAa,MAAM;;;;ACEnB,QAAQ,IAAI,IAAI"`,

@IWANABETHATGUY IWANABETHATGUY Jul 10, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have verified the sourcemap is correct.
image

@IWANABETHATGUY
IWANABETHATGUY marked this pull request as ready for review July 10, 2024 03:05
@IWANABETHATGUY
chore: 🤖 init
0481325
@IWANABETHATGUY
chore: 🤖 update
c435721 
chore: 🤖 update

chore: 🤖 update

chore: 🤖 update

chore: 🤖 update

chore: 🤖 update

chore: 🤖 update

chore: 🤖 update
@IWANABETHATGUY
IWANABETHATGUY force-pushed the fix/multi-declaration-tree-shake branch from 60d2afd to c435721 Compare July 10, 2024 03:05
@IWANABETHATGUY
IWANABETHATGUY enabled auto-merge July 10, 2024 03:06
@IWANABETHATGUY
IWANABETHATGUY added this pull request to the merge queue Jul 10, 2024
@IWANABETHATGUY
IWANABETHATGUY marked this pull request as draft July 10, 2024 03:15
auto-merge was automatically disabled July 10, 2024 03:15

Pull request was converted to draft

@IWANABETHATGUY
IWANABETHATGUY marked this pull request as ready for review July 10, 2024 03:16
Merged via the queue into main with commit 0cf1c22 Jul 10, 2024
@IWANABETHATGUY
IWANABETHATGUY deleted the fix/multi-declaration-tree-shake branch July 10, 2024 03:23
@GulgDev GulgDev mentioned this pull request Jul 7, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hyfdev hyfdev hyfdev approved these changes

Assignees

No one assigned

Labels

needs-triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@IWANABETHATGUY @hyfdev