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.

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

fix: variable undefined with multiple same import #4356

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

ahabhgk
Copy link
Collaborator

@ahabhgk ahabhgk commented Oct 18, 2023

Summary

checkout added test case

after we introduce source_order for new tree shaking implementation, HarmonyImportSideEffectDependency will be duplicated, and merged by InitFragment, this is aligned behavior with webpack, before it won't be duplicated

// before:
import {a} from "./lib" // => Dependency(100 /* dependency_id */, "./lib")
import {b} from "./lib" // => Dependency(100 /* dependency_id */, "./lib")
// after:
import {a} from "./lib" // => Dependency(100 /*dependency_id*/, "./lib", 0 /* source_order */)
import {b} from "./lib" // => Dependency(101 /*dependency_id*/, "./lib", 1 /* source_order */)

but in the old tree shaking implementation will find dependency by its request and dependency_type, so when we want to find Dependency(101), we actually will find Dependency(100), if Dependency(100) is unused, then we won't generate the code, so Dependency(101) will be removed mistakenly

Test Plan

  • packages/rspack/tests/configCases/tree-shaking/multiple-same-import-side-effect

Require Documentation?

  • No
  • Yes, the corresponding rspack-website PR is __

@github-actions github-actions bot added release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack. labels Oct 18, 2023
@ahabhgk ahabhgk linked an issue Oct 18, 2023 that may be closed by this pull request
IWANABETHATGUY
IWANABETHATGUY previously approved these changes Oct 18, 2023
@ahabhgk ahabhgk added this pull request to the merge queue Oct 18, 2023
Merged via the queue into main with commit d9d192e Oct 18, 2023
14 checks passed
@ahabhgk ahabhgk deleted the fix-multiple-same-import-side-effect-dep branch October 18, 2023 10:44
@ahabhgk
Copy link
Collaborator Author

ahabhgk commented Oct 18, 2023

!canary

@github-actions
Copy link
Contributor

0.3.7-canary-beb5892-20231018143949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix variable undefined when multiple same import
2 participants