Changes UMD callsite to be more likely to pass in the intended object. #10477

MicahZoltu · 2019-09-21T11:50:28Z

Q	A
Fixed Issues?	Fixes #10476
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	No
Documentation PR Link
Any Dependency Changes?
License	CC0/Unlicense/MIT/WTFPL/whatever you want.

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.

nicolo-ribaudo · 2019-09-23T17:53:46Z

packages/babel-plugin-transform-modules-umd/src/index.js

@@ -27,7 +27,7 @@ const buildWrapper = template(`

      GLOBAL_TO_ASSIGN;
    }
-  })(this, function(IMPORT_NAMES) {
+  })(globalThis || window || this, function(IMPORT_NAMES) {


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

Suggested change

})(globalThis || window || this, function(IMPORT_NAMES) {

})(

typeof globalThis === "object" ? globalThis

: typeof window === "object" ? window

: this,

function(IMPORT_NAMES) {

Definitely need Nicolo's suggestion, but we should use self instead of window to support workers as well.

typeof globalThis === "object" ? globalThis : typeof self === "object" ? self : typeof window === "object" ? window : typeof global === "object" ? global : this

Is 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?

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.

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.

nicolo-ribaudo · 2019-09-23T18:03:31Z

After the code changes, you can run make bootstrap and OVERWRITE=true make test to update the failing tests

MicahZoltu · 2019-09-24T02:58:13Z

After the code changes, you can run make bootstrap and OVERWRITE=true make test to update the failing tests

Unfortunately I'm on Windows, so I don't have make.

MicahZoltu · 2019-09-24T03:00:10Z

Also the pre-commit hook is failing on my machine in what appears to be unrelated code, so the current commit is pushed with --no-verify.

jridgewell · 2019-09-24T03:36:40Z

Pushed updated tests.

babel-bot · 2019-09-24T03:42:04Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11679/

jridgewell · 2019-09-24T03:49:48Z

Actually, do we even need a globalThis check? The global parameter is only used if we're not using requirejs and we're not in node. That means we're in browser mode, and self will be enough.

MicahZoltu · 2019-09-24T04:12:43Z

globalThis is intended to be the future of the universal global object. It feels like a good idea to try that first, as one day (far, far in the future) window, self, and/or global may be deprecated.

Glancing at the globalThis docs on MDN it also references frames in some situations, not sure if we need to or should handle that as well?

JLHwung · 2019-10-15T18:10:19Z

Unfortunately I'm on Windows, so I don't have make.

gnuwin32 works for me on Windows.

MicahZoltu · 2019-10-15T19:12:53Z

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?

jridgewell · 2019-10-15T19:18:51Z

I kicked the build servers. It definitely shouldn't take this long.

MicahZoltu · 2019-10-18T05:18:53Z

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?

MicahZoltu · 2019-10-24T13:20:57Z

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.

jridgewell

Passed this time.

MicahZoltu · 2019-10-28T05:59:37Z

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?

jridgewell · 2019-10-28T06:25:25Z

Waiting on @nicolo-ribaudo to merge before the next appropriate release.

nicolo-ribaudo · 2019-10-28T07:36:50Z

(Soon)

nicolo-ribaudo added area: modules PR: New Feature 🚀 A type of pull request used for our changelog categories labels Sep 21, 2019

nicolo-ribaudo reviewed Sep 23, 2019

View reviewed changes

jridgewell closed this Oct 15, 2019

jridgewell reopened this Oct 15, 2019

MicahZoltu and others added 2 commits October 24, 2019 15:31

Update tests

20b4f43

nicolo-ribaudo approved these changes Oct 24, 2019

View reviewed changes

jridgewell approved these changes Oct 24, 2019

View reviewed changes

nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Oct 24, 2019

nicolo-ribaudo added this to the v7.7.0 milestone Oct 25, 2019

nicolo-ribaudo merged commit 198b4a0 into babel:master Oct 29, 2019

MicahZoltu deleted the patch-1 branch October 29, 2019 21:41

MicahZoltu mentioned this pull request Oct 29, 2019

Does not work when imported as a module. mozilla/webextension-polyfill#202

Closed

JLHwung mentioned this pull request Nov 13, 2019

Circumvent typeof transform for umd build template #10701

Merged

snyk-bot mentioned this pull request Nov 14, 2019

[Snyk] Upgrade @babel/runtime from 7.6.3 to 7.7.1 cds-snc/report-a-cybercrime#936

Merged

This was referenced Nov 28, 2019

[Snyk] Upgrade @babel/runtime from 7.0.0-beta.42 to 7.7.2 ali8889/mist#4

Open

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 dyna-dot/CyberChef#3

Open

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade @babel/runtime from 7.3.1 to 7.7.4 dyna-dot/bit-javascript#5

Open

rpl mentioned this pull request Dec 11, 2019

fix: Update babel deps to babel 7 mozilla/webextension-polyfill#214

Merged

snyk-bot mentioned this pull request Dec 12, 2019

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 jakeherp/todo-list#1

Merged

snyk-bot mentioned this pull request Jan 12, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.7.7 mboocannon/msgraph-sdk-javascript#1

Open

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 29, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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. #10477

MicahZoltu commented Sep 21, 2019 •

edited

nicolo-ribaudo Sep 23, 2019

nicolo-ribaudo Sep 23, 2019

jridgewell Sep 23, 2019

MicahZoltu Sep 24, 2019 •

edited

jridgewell Sep 24, 2019

MicahZoltu Sep 24, 2019

nicolo-ribaudo commented Sep 23, 2019

MicahZoltu commented Sep 24, 2019

MicahZoltu commented Sep 24, 2019

jridgewell commented Sep 24, 2019

babel-bot commented Sep 24, 2019

jridgewell commented Sep 24, 2019

MicahZoltu commented Sep 24, 2019 •

edited

JLHwung commented Oct 15, 2019

MicahZoltu commented Oct 15, 2019

jridgewell commented Oct 15, 2019

MicahZoltu commented Oct 18, 2019

MicahZoltu commented Oct 24, 2019

jridgewell left a comment

MicahZoltu commented Oct 28, 2019

jridgewell commented Oct 28, 2019

nicolo-ribaudo commented Oct 28, 2019

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. #10477

Conversation

MicahZoltu commented Sep 21, 2019 • edited

nicolo-ribaudo Sep 23, 2019

Choose a reason for hiding this comment

nicolo-ribaudo Sep 23, 2019

Choose a reason for hiding this comment

jridgewell Sep 23, 2019

Choose a reason for hiding this comment

MicahZoltu Sep 24, 2019 • edited

Choose a reason for hiding this comment

jridgewell Sep 24, 2019

Choose a reason for hiding this comment

MicahZoltu Sep 24, 2019

Choose a reason for hiding this comment

nicolo-ribaudo commented Sep 23, 2019

MicahZoltu commented Sep 24, 2019

MicahZoltu commented Sep 24, 2019

jridgewell commented Sep 24, 2019

babel-bot commented Sep 24, 2019

jridgewell commented Sep 24, 2019

MicahZoltu commented Sep 24, 2019 • edited

JLHwung commented Oct 15, 2019

MicahZoltu commented Oct 15, 2019

jridgewell commented Oct 15, 2019

MicahZoltu commented Oct 18, 2019

MicahZoltu commented Oct 24, 2019

jridgewell left a comment

Choose a reason for hiding this comment

MicahZoltu commented Oct 28, 2019

jridgewell commented Oct 28, 2019

nicolo-ribaudo commented Oct 28, 2019

MicahZoltu commented Sep 21, 2019 •

edited

MicahZoltu Sep 24, 2019 •

edited

MicahZoltu commented Sep 24, 2019 •

edited