-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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: mangle with destructuring #18319
fix: mangle with destructuring #18319
Conversation
For maintainers only:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @vankop
here is better to use "webpack" design. we should introduce new hook for destructuring assignment in JavascriptParser then just add handler in HarmonyParser like: parser.hooks.destructuringAssignment
.for(harmonySpecifierTag)
.tap("HarmonyImportDependencyParserPlugin", handler); also we should not touch dependencies here.. just mark usage ( not sure here ) |
btw we dont need to bail out mangling ( only when we can't compute properties ), in dependencies we can pass range that we will change to something like |
@vankop I tried to refactor this but failed, I'm not very familiar with the code related to the |
@ahabhgk Thanks for your update. I labeled the Pull Request so reviewers will review it again. @alexander-akait Please review the new changes. |
Figured out why before failed, made a refactor, now the logic of replacing the destructuring assignment an import specifier is handled by import * as module from "./module"
const { a, b } = module
//--------------------- will be replaced by HarmonyDestructuredImportSpecifierDependency
// and `"mangledExportHere": a` will also be done by this dependency But I am still confused about why we need to introduce the new |
/cc @vankop |
so.. based on #16941 pull request. what we need to do here:
|
in render part we just need to add ( dont change existing code ) something like: if (dep. referencedPropertiesInDestructuring)
for (const property of dep. referencedPropertiesInDestructuring) source.replace(...) |
if you want I can finish this PR @ahabhgk |
@vankop Yes, feel free to change on this PR |
@vankop I did some refactoring based on the third point, it turns out indeed there is no need to add a new dependency for replacing, would you take an another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everything else LGTM
/cc @ahabhgk Can you look at this soon? I want to make a release this week, so my roadmap is:
|
@alexander-akait I was just looking at this when you send the message, I think this should be fine now, except one last thing need confirm from @vankop. |
Thank youo |
fix #18278
What kind of change does this PR introduce?
introduce
DestructuringAssignmentPropertyKeyDependency
to replace the key in destructuring instead of bailout the mangleExports in some condition.for example in #18095, we will bailout the mangleExports (
canMangle: false
) when the module is a reexport of another module, but in this PR, we will no longer bailout:index.js will generate:
and since the reexport no longer bailout the mangleExports, this should make the output size smaller in some cases.
Did you add tests for your changes?
added
Does this PR introduce a breaking change?
no
What needs to be documented once your changes are merged?
none