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: handle star import name collisions in module concatenation #11751
Conversation
For maintainers only:
|
The failing pipelines seem to be related due to change of generated JS. Not all tests run successfully on Windows, so I can't update snapshots reliably at this point. |
@Knagis What is tests not working? Do you read |
one example:
another:
there were few more. Most tests do run without problems. I didn't try running under WSL yet since I am in VM where it does not work. |
@Knagis Yep, look we need some improvement for our tests, maybe you can help? I think it should be not hard, problem with |
I might be able, yes. I think I might have something in few days. In the meantime for now I am updating snapshots using WSL which hopefully will enable this PR to be green |
d39747f
to
a014fbb
Compare
a014fbb
to
95eabe4
Compare
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
@@ -999,7 +999,7 @@ class ConcatenatedModule extends Module { | |||
const name = reference.identifier.name; | |||
if (ConcatenationScope.isModuleReference(name)) { | |||
const match = ConcatenationScope.matchModuleReference(name); | |||
if (!match || match.ids.length < 1) continue; |
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.
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.
findNewName
should already return a name that avoids collisions with other variables. But the actual bug was that in some cases potential collisions were not detected. This happened in the case when ids = []
, which is the case for namespace objects or the externals (which are also treated as namespace objects).
The above snippet fixes the problem.
Thanks |
What kind of change does this PR introduce?
Fixes #11743
Did you add tests for your changes?
Yes
Does this PR introduce a breaking change?
No
What needs to be documented once your changes are merged?
Nothing