Changes UMD callsite to be more likely to pass in the intended object.#10477
Changes UMD callsite to be more likely to pass in the intended object.#10477nicolo-ribaudo merged 2 commits intobabel:masterfrom MicahZoltu:patch-1
Conversation
| GLOBAL_TO_ASSIGN; | ||
| } | ||
| })(this, function(IMPORT_NAMES) { | ||
| })(globalThis || window || this, function(IMPORT_NAMES) { |
There was a problem hiding this comment.
Wouldn't this throw a globalThis is not defined error in browsers with modules supprot (where the code is executed in strict mode) but without globalThis support?
Those browsers are Edge 16, Firefox 60-64, Chrome 61-70, Safari 10.1-12.0 and Opera 48-57
There was a problem hiding this comment.
| })(globalThis || window || this, function(IMPORT_NAMES) { | |
| })( | |
| typeof globalThis === "object" ? globalThis | |
| : typeof window === "object" ? window | |
| : this, | |
| function(IMPORT_NAMES) { |
There was a problem hiding this comment.
Definitely need Nicolo's suggestion, but we should use self instead of window to support workers as well.
There was a problem hiding this comment.
typeof globalThis === "object" ? globalThis
: typeof self === "object" ? self
: typeof window === "object" ? window
: typeof global === "object" ? global
: thisIs that too overboard on trying? I think there are environments where window is defined but not self or globalThis (older browsers), and older versions of NodeJS only define global I believe. Does it make sense to fallback to this at the end? Perhaps for test environments the this fallback is valuable?
There was a problem hiding this comment.
I think just self is enough. IE has supported it forever. As for global, let's assume people can just load a globalThis polyfill in node. Client side we have to worry about bytes.
There was a problem hiding this comment.
The suggested changes have been applied and the code rebased. Anyone can feel free to takeover and finish this up (e.g., update the tests). I'm on Windows and this repo appears to have a number of issues with Windows so I doubt I'll be able to finish it.
|
After the code changes, you can run |
Unfortunately I'm on Windows, so I don't have |
|
Also the pre-commit hook is failing on my machine in what appears to be unrelated code, so the current commit is pushed with |
|
Pushed updated tests. |
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11679/ |
|
Actually, do we even need a |
|
Glancing at the |
gnuwin32 works for me on Windows. |
|
Does the PR build process really take over 21 days to complete, or is it stalled? If it is stalled, what can be done to restart it so we can move forward with this PR? |
|
I kicked the build servers. It definitely shouldn't take this long. |
|
The Travis build failure does not appear to be related to the changes I have made in this PR. Can someone more familiar with this project glance at the build failure and help me understand how they are related to these changes if they are? |
|
The build failure doesn't appear to have anything with these changes. Can I get someone familiar with the project to validate that claim? |
Fixes #10476 Note: This PR should be considered pseudocode and used as an illustration of the proposed fix. I do not know nearly enough about this project to know if this is an appropriate solution to the problem, nor do I have the confidence to update the tests appropriately.
|
Is there anything I can or should do to help move this along, or is it just a matter of waiting for it to get pickup up in a release? |
|
Waiting on @nicolo-ribaudo to merge before the next appropriate release. |
|
(Soon) |
See #10476 for details.
Note: This PR should be considered pseudocode and used as an illustration of the proposed fix. I do not know nearly enough about this project to know if this is an appropriate solution to the problem, nor do I have the confidence to update the tests appropriately.