-
-
Notifications
You must be signed in to change notification settings - Fork 5.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 transform of named import with shadowed namespace import #15898
Conversation
dhlolo
commented
Aug 27, 2023
•
edited by nicolo-ribaudo
Loading
edited by nicolo-ribaudo
Q | A |
---|---|
Fixed Issues? | Fixes #15879 |
Patch: Bug Fix? | |
Major: Breaking Change? | |
Minor: New Feature? | |
Tests Added + Pass? | Yes |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/55416/ |
Thank you! Could you add some tests in If it is the first time that you contribute to Babel, follow these steps: (you need to have
|
Hi, nicola, I updated code and add a input.mjs for test. But expected output is to throw an Error for now. I don't know how to make jest know that the result of my input.mjs should be such an error in babel system. |
Can this temp PR be merged and pub a patch version for now ? As our team met this problem in production for twice. |
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.
Instead of throwing an error, could you try fixing this issue by updating this check to be "if there is at least one namespace import and zero other imports"?
babel/packages/babel-helper-module-transforms/src/normalize-and-load-metadata.ts
Line 150 in 2c07574
if (metadata.importsNamespace.size > 0) { |
Also, you could update the line below to use destructuring as described in the comment in the code, since we don't compile it as loose anymore :)
Thx for your advice. It is done. And to be on the safe side, I do not revert the changes of throwing error. |
Since we have now tests to make sure that it works, I would prefer to revert it :) It adds quite a bit of noise to the code, just for an error that is never expected to happen. |
It sounds reasonable. It is reverted. |
d1511fe
to
60af27f
Compare
...ransform-modules-commonjs/test/fixtures/shadowed-namespace-import/transformed-name/output.js
Outdated
Show resolved
Hide resolved
packages/babel-plugin-transform-modules-commonjs/test/fixtures/regression/lazy-7176/output.js
Outdated
Show resolved
Hide resolved
687103a
to
95b8db6
Compare
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.
Thanks!
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.
Thank you! :)