Skip to content
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

Define regeneratorRuntime globally as a side-effect, again. #369

Merged
merged 4 commits into from
Mar 7, 2019

Conversation

benjamn
Copy link
Collaborator

@benjamn benjamn commented Mar 7, 2019

In #353, I simplified the implementation of regenerator-runtime by converting runtime.js to a normal CommonJS module. No more global object detection, and no more sneaky capture/deletion of regeneratorRuntime.

Unfortunately, folks who use only @babel/polyfill and not @babel/transform-runtime may have existing code that relies on a global regeneratorRuntime variable. That code will eventually disappear (🤞), but in the meantime we can continue supporting it by simply ensuring that regeneratorRuntime is defined globally as a side-effect of importing regenerator-runtime.

This turns out to be much easier than detecting the global object, as long as runtime.js runs as a non-strict CommonJS module. See my code comments for another fallback solution that should cover all remaining cases.

Background discussion: babel/babel#7646 (comment)

Fixes #363.
Fixes #337.
Fixes #367.

benjamn added 2 commits March 7, 2019 09:56
In #353, I simplified the implementation of regenerator-runtime by
converting runtime.js to a normal CommonJS module. No more global object
detection, and no more sneaky capture/deletion of regeneratorRuntime.

Unfortunately, folks who use only @babel/polyfill and not
@babel/transform-runtime may have existing code that relies on a global
regeneratorRuntime variable. That code will eventually disappear
(:crossed-fingers:), but in the meantime we can continue supporting it by
simply ensuring that regeneratorRuntime is defined globally as a
side-effect of importing regenerator-runtime.

This turns out to be much easier than detecting the global object, as long
as runtime.js runs as a non-strict CommonJS module. See my code comments
for another fallback solution that should cover all remaining cases.

Background discussion:
babel/babel#7646 (comment)

Fixes #363.
Fixes #337.
Fixes #367.
@@ -10,6 +10,7 @@
"generator",
"async"
],
"sideEffects": true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to keep Webpack from removing import … from "regenerator-runtime", since the package now has an important side-effect.

@benjamn benjamn merged commit 063f14e into master Mar 7, 2019
@benjamn benjamn deleted the define-regeneratorRuntime-globally-again branch March 7, 2019 20:22
@zloirock
Copy link
Contributor

zloirock commented Mar 7, 2019

I can't understand, why you against simple global object detection like this? With this code, you will have fewer issues than with code from this PR - now someone definitely will try to load it in strict mode.

@benjamn
Copy link
Collaborator Author

benjamn commented Mar 7, 2019

You're throwing around words like "simple" and "definitely" based on your intuition, and I think you reasoning may be biased because you have your own implementation.

I prefer this approach because it doesn't involve obtaining a reference to the global object. I've been down that road before (e.g. #336), and I don't care to go there again.

All the identifiers you're checking in global.js could be shadowed by malicious code, and you're falling back to Function("return this")() for Node.js, which doesn't have window or self.

If someone happens to load this code in strict mode, that's fine. It will fall back to Function("r", "regeneratorRuntime = r")(runtime), which escapes strict mode. I don't even think that fallback is necessary, since loading this code in strict mode is a bug, but this implementation covers that case too, just to be safe.

If you're wondering why I didn't just depend on core-js here… this code has no other dependencies, which is important because it gets used in non-module contexts, such as the command-line regenerator script.

@zloirock
Copy link
Contributor

zloirock commented Mar 7, 2019

You're throwing around words like "simple" and "definitely" based on your intuition, and I think you reasoning may be biased because you have your own implementation.

It's based on the experience of support modules which need the same functionality and many issues in the babel repo related strict mode. A long time ago core-js had much more issues with the global object reference with many interesting environments than regenerator with #336, but the proposed code does not cause any issues more than 3 years.

It will fall back to Function..., which escapes strict mode.

And will cause a CSP issue in strict mode.

If you're wondering why I didn't just depend on core-js here…

I understand it and I do not insist - core-js also hasn't any dependencies for many reasons.

This PR is a solution to my issue, but I told about global object since just wanted to prevent some future issues.

@MattiasBuelens
Copy link

With this change, @babel/plugin-transform-runtime would also pollute the global scope with a regeneratorRuntime variable, which kind of defeats the purpose of that plugin. 😕

I think we should go back to having two versions: runtime.js (for browsers) and runtime-module.js (for CommonJS). The problem with this original approach was runtime-module.js needed to restore the old version of regeneratorRuntime after it had require'd runtime.js (source).

What if we turned it around?

  • runtime-module.js contains the actual implementation. It defines a local variable runtime and (if available) exports it to module.exports. This is basically lines 1 to 711 from runtime.js.
  • runtime.js is generated by a script that concatenates runtime-module.js together with an "outro" that defines the global regeneratorRuntime variable (lines 713 to 726). (We can't require("./runtime-module.js") since that wouldn't work inside a browser.) The result would be identical to the current version of runtime.js.

Usage:

  • CommonJS users would look at the main field of package.json to load runtime-module.js.
  • Browser users would need to require("regenerator-runtime/runtime"), as they did before version 0.13.0. (Alternatively, we could add a browser field in package.json for module loaders that support it.)

@OurMajesty
Copy link

For some reason regenerator-runtime@latest is 0.13.1
изображение

@benjamn
Copy link
Collaborator Author

benjamn commented Mar 20, 2019

@OurMajesty That's right, I didn't want people updating to this version accidentally before we were sure the changes worked.

@MattiasBuelens With that solution, I would be worried that people would end up bundling both runtime-module.js and runtime.js, thus duplicating the code. Compared to that very real risk, I don't see the "pollution" of global scope with an additional variable as much of a problem. The purpose of the runtime transform, in my mind, is not so much to avoid polluting global scope, but to ensure you don't have to import/bundle regenerator-runtime if you don't use any generator functions (the "pay for what you use" principle). That property still holds with (or without) this change, for applications that use @babel/plugin-transform-runtime. This change simply makes @babel/polyfill work the way it previously did, for applications that still use that approach. The global variable is a small price to pay for backwards compatibility.

@MattiasBuelens
Copy link

The purpose of the runtime transform, in my mind, is not so much to avoid polluting global scope, but to ensure you don't have to import/bundle regenerator-runtime if you don't use any generator functions (the "pay for what you use" principle).

Well, the documentation for @babel/plugin-transform-runtime recommends it for library development because it should not pollute the global scope:

Another purpose of this transformer is to create a sandboxed environment for your code. If you use @babel/polyfill and the built-ins it provides such as Promise, Set and Map, those will pollute the global scope. While this might be ok for an app or a command line tool, it becomes a problem if your code is a library which you intend to publish for others to use or if you can't exactly control the environment in which your code will run.

Sure, an extra global variable doesn't matter when you're bundling a web app. However, if you're bundling a web library, and users use your library inside their own web app that uses @babel/polyfill, it could mean that users accidentally use your version of the runtime instead of the one from their polyfill.

Of course, in this case there's also the problem of bundling the runtime twice (once in the library, once in the app), so I guess library developers should recommend their users to bundle the library's dependencies themselves...? 😕

Either way, I understand you don't want to risk bundling the runtime twice. I think it's unfortunate for library developers, but I guess that's the trade-off we have to make.

@zloirock
Copy link
Contributor

@benjamn could you publish a release with those changes as latest? Without it, we will have problems since babel@7.4 which requires it already released.

@nicolo-ribaudo
Copy link
Contributor

Well, Babel downloads the correct version anyway because it uses ^0.13.2

@zloirock
Copy link
Contributor

@nicolo-ribaudo I mean the case of separate usage of core-js and regenerator-runtime from the latest branch in preset-env.

@benjamn
Copy link
Collaborator Author

benjamn commented Mar 20, 2019

I just set 0.13.2 to be latest with the following command:

npm dist-tag add regenerator-runtime@0.13.2 latest

The next tag still points to that version, too, in case anyone was relying on that.

wincent added a commit to wincent/js that referenced this pull request Mar 31, 2019
Seeing a lot of:

    _Object$defineProperty is not a function

Scanning the release notes:

    https://github.com/babel/babel/releases

This one stands out:

    https://github.com/babel/babel/releases/tag/v7.4.0

Specifically, "Update to core-js@3":

    babel/babel#7646

There are 133 comments which I confess to not having read, and various
offshoots to places like:

    babel/babel#9442

and:

    facebook/regenerator#369

I did find this enormous update doc, but haven't found the answer in
there yet and the Babel docs themselves don't appear to have been
updated:

    https://github.com/zloirock/core-js/blob/master/docs/2019-03-19-core-js-3-babel-and-a-look-into-the-future.md

Although this blog post is briefer and covers a few things:

    https://babeljs.io/blog/2019/03/19/7.4.0

In the end the build works if I get rid of
`@babel/plugin-transform-runtime`, which doesn't seem to play too well
in conjunction with `@babel/preset-env`, at least in this set-up.
Macil added a commit to InboxSDK/InboxSDK that referenced this pull request May 17, 2019
Macil added a commit to InboxSDK/InboxSDK that referenced this pull request May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants