-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fix crash on umd and module merge, allow umds to be accessed when merged with a non-UMD symbol #28782
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
Conversation
…ged with a non-UMD symbol
@@ -38,13 +34,5 @@ | |||
} | |||
|
|||
a.x + a.y + a.z + a.conflict; | |||
~ |
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.
Wasn't that error correct?
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.
Not really, no. If there's a global augmentation adding a thing to the global scope, then it should be globally accessible, period.
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.
But aren't the globals accessible from module? They are only accessible from non module file?
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.
Today, the umd declaration poisons the symbol unconditionally (irrespective of other things the symbol may have merged with). With this change, the UMD declaration only errors on access if every declaration of the symbol is a UMD declaration.
Works for me 😄 |
@DanielRosenwasser since this fixes a crash, do we want to port it to 3.2? |
This is an active 3.2 blocker for a few of our repos so I'd very much appreciate it 😄 |
+1 this is a blocker for my project as well, much appreciated if this fix can be released soon. |
Yesterday's nightly should've included the fix, so installing |
Thanks, works for me 😀 |
Fixes #28762