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

Update regenerator-runtime to latest version #9442

Merged
merged 1 commit into from Mar 19, 2019
Merged

Update regenerator-runtime to latest version #9442

merged 1 commit into from Mar 19, 2019

Conversation

MattiasBuelens
Copy link
Contributor

Q                       A
Fixed Issues? facebook/regenerator#336 facebook/regenerator#346 facebook/regenerator#353 facebook/regenerator#355 #1286
Patch: Bug Fix? Yes
Major: Breaking Change? Possibly, but probably not (facebook/regenerator#353)
Minor: New Feature? No
Tests Added + Pass? Existing tests pass
Documentation PR Link No
Any Dependency Changes? regenerator-runtime (minor)
License MIT

Update regenerator-runtime from version 0.12.0 to the latest version 0.13.1.

facebook/regenerator#353 changes how the runtime defines the regeneratorRuntime global variable (when loaded as a script) or its module exports (when loaded as a CommonJS module). This obsoletes a previous attempt (facebook/regenerator#346) at fixing a CSP violation (facebook/regenerator#336). This change is a minor bump for regenerator-runtime (because it no longer has a separate runtime-module.js), but I don't think it will affect Babel users.

facebook/regenerator#355 fixes a regression where IE8 would throw a parse error when loading the runtime (#1286).

@wmertens
Copy link

wmertens commented Feb 1, 2019

Unless something around this code changed since November, I can say that it will break the build, because Babel relies on the polyfill being a global, IIRC.

How I found out: I use pnpm which enforces strict package dependencies, and this caused 0.13 to be exposed to my babel builds which work with 0.12. This broke the build.

I may be wrong, but that's basically what happened: I used 0.13 with Babel 7.something, and things broke.

@MattiasBuelens
Copy link
Contributor Author

You're right, the tests are failing. It looks like my local yarn install didn't pick up the dependency update before I ran the tests.

I guess @babel/polyfill should define the global then? 😕

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 1, 2019

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

@MattiasBuelens
Copy link
Contributor Author

Hmm, it looks like @babel/preset-env also needs to be updated to generate:

var regeneratorRuntime = require('regenerator-runtime/runtime');

instead of just:

require('regenerator-runtime/runtime')

as reported in #9205.

@benjamn @loganfsmyth Is it okay if I continue with this? Or do you think I should use a different approach?

@danez
Copy link
Member

danez commented Mar 1, 2019

Sorry for the long silence.

The changes look good to me, but yes we need to update preset-env and if I'm not mistaken we also need to update the generation in babel-runtime. Would you be willing to do this?

Also I wonder what we should do in polyfill if the regenerator runtime does already exists on the global object? The old version of the regenerator-runtime was not overwriting and silently not doing anything if imported a second time. Should we do the same or trigger a warning?

@zloirock
Copy link
Member

zloirock commented Mar 2, 2019

global is not available in browsers, globalThis still is a proposal and not available even with polyfills of stable features.

This way of regenerator-runtime import will cause a serious problem for preset-env. #7646 mean separate inclusion of core-js and regenerator-runtime and deprecation of @babel/polyfill. preset-env works just with import 'regenerator-runtime/runtime'.

@MattiasBuelens
Copy link
Contributor Author

The changes look good to me, but yes we need to update preset-env and if I'm not mistaken we also need to update the generation in babel-runtime. Would you be willing to do this?

Sure, I could help with that. I'm just not entirely certain on what approach we'd want to take, see below.

Also I wonder what we should do in polyfill if the regenerator runtime does already exists on the global object? The old version of the regenerator-runtime was not overwriting and silently not doing anything if imported a second time. Should we do the same or trigger a warning?

Good point. I'd say we should not overwrite the global if it already exists, so you can load a modified version of regenerator-runtime if necessary. I don't know if we need to warn about that.

global is not available in browsers, globalThis still is a proposal and not available even with polyfills of stable features.

Is this about this line?

global.regeneratorRuntime = regeneratorRuntime;

I'm pretty sure that CommonJS global gets transpiled to something browser-friendly like:

typeof global !== "undefined" ? global : typeof self !== "undefined" ? self : typeof window !== "undefined" ? window : {}

Otherwise, global._babelPolyfill wouldn't work either.

This way of regenerator-runtime import will cause a serious problem for preset-env. #7646 mean separate inclusion of core-js and regenerator-runtime and deprecation of @babel/polyfill. preset-env works just with import 'regenerator-runtime/runtime'.

I hadn't seen your pull request yet, looks very interesting! But indeed, with this new version of regenerator-runtime, preset-env can no longer rely on it to define regeneratorRuntime globally if it's used as an import.

Do we need a wrapper around regenerator-runtime that ensures regeneratorRuntime is always globally defined, and import that from polyfill and preset-env instead? 🤔

For useBuiltIns: 'usage', preset-env could insert import regeneratorRuntime from 'regenerator-runtime/runtime' instead so we don't need to define a global. Never mind, we have transform-runtime for that.

Alternatively, we update regenerator-runtime to always define the global, as it was before... 😕

@zloirock
Copy link
Member

zloirock commented Mar 2, 2019

I'm pretty sure that CommonJS global gets transpiled to something browser-friendly like:

See this issue.

Do we need a wrapper around regenerator-runtime that ensures regeneratorRuntime is always globally defined, and import that from polyfill and preset-env instead?

Since now we haven't standard global object reference, without entry point which defines regenerator runtime globally, preset-env will have serious problems.

usage mode of preset-env should add global regeneratorRuntime. We can imports globalThis from core-js and use it or use import regeneratorRuntime from 'regenerator-runtime';. It's a complication of preset-env, but not something critically hard.

But what should do entries mode? It should remove regenerator-runtime/runtime import if no one plugin which uses regenerator transforms used in the target environment. It's not a problem to remove

import 'regenerator-runtime/runtime';

but how we can detect that we should remove

import regeneratorRuntime from 'regenerator-runtime';
Function('return this')().regeneratorRuntime = regeneratorRuntime;

?

Moreover, since we deprecate @babel/polyfill, we can without any serious problems explain to users that they instead of @babel/polyfill should use

import 'core-js/stable';
import 'regenerator-runtime/runtime';

but not

import 'core-js/stable';
import 'core-js/features/global-this';
import regeneratorRuntime from 'regenerator-runtime';
globalThis.regeneratorRuntime = regeneratorRuntime;

@danez danez mentioned this pull request Mar 4, 2019
@benjamn
Copy link
Contributor

benjamn commented Mar 7, 2019

Can we hold this PR until facebook/regenerator#369 is released?

@MattiasBuelens
Copy link
Contributor Author

@benjamn Sure! I think whatever solution we come up with for Babel would involve defining regeneratorRuntime globally anyway. So it'd be better if regenerator-runtime did that for us. 🙂

@@ -12,6 +12,6 @@
"main": "lib/index.js",
"dependencies": {
"core-js": "^2.5.7",
"regenerator-runtime": "^0.12.0"
"regenerator-runtime": "^0.13.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"regenerator-runtime": "^0.13.1"
"regenerator-runtime": "^0.13.2"

Version 0.13.2 includes facebook/regenerator#369, which defines regeneratorRuntime globally without needing to detect the global object, using these two techniques.

@@ -10,7 +10,7 @@
"author": "Sebastian McKenzie <sebmck@gmail.com>",
"dependencies": {
"core-js": "^2.5.7",
"regenerator-runtime": "^0.12.0"
"regenerator-runtime": "^0.13.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"regenerator-runtime": "^0.13.1"
"regenerator-runtime": "^0.13.2"

@@ -9,7 +9,7 @@
"repository": "https://github.com/babel/babel/tree/master/packages/babel-runtime",
"author": "Sebastian McKenzie <sebmck@gmail.com>",
"dependencies": {
"regenerator-runtime": "^0.12.0"
"regenerator-runtime": "^0.13.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"regenerator-runtime": "^0.13.1"
"regenerator-runtime": "^0.13.2"

@@ -25,4 +25,5 @@ if (global._babelPolyfill && typeof console !== "undefined" && console.warn) {
);
}

global.regeneratorRuntime = regeneratorRuntime;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should no longer be necessary, though it also won't do any harm.

@@ -14,4 +14,5 @@ import "core-js/fn/promise/finally";
// Ensure that we polyfill ES6 compat for anything web-related, if it exists.
import "core-js/web";

import "regenerator-runtime/runtime";
import regeneratorRuntime from "regenerator-runtime/runtime";
global.regeneratorRuntime = regeneratorRuntime;
Copy link
Contributor

Choose a reason for hiding this comment

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

The old import "regenerator-runtime/runtime" should work fine here, too.

@zloirock
Copy link
Member

zloirock commented Mar 7, 2019

Cause of facebook/regenerator#369, I bumped regenerator-runtime version in #7646 to ^0.13.2 in all required places.

@MattiasBuelens
Copy link
Contributor Author

Updated to version 0.13.2.

However, I think there's still a problem with the new approach. The new regenerator-runtime will always define regeneratorRuntime globally, even when used through @babel/plugin-transform-runtime. This kind of defeats the purpose of the runtime transform. 😕

I've suggested an alternative approach in a comment on facebook/regenerator#369. I would love to hear your thoughts on this. 🙂

@nicolo-ribaudo
Copy link
Member

I agree with you. Ideally the global variable assignment should be opt-in (or at least it should be possible to opt out).
It shouldn't be a blocker for this to be merged, since we just generate import "regenerator-runtime/runtime" anyway, right?

@MattiasBuelens
Copy link
Contributor Author

It shouldn't be a blocker for this to be merged, since we just generate import "regenerator-runtime/runtime" anyway, right?

Correct. Sure, if it's okay that @babel/plugin-transform-runtime defines a global regeneratorRuntime for now, then we can already land this.

@nicolo-ribaudo nicolo-ribaudo merged commit 60005b3 into babel:master Mar 19, 2019
@MattiasBuelens MattiasBuelens deleted the update-regenerator-runtime branch March 19, 2019 22:32
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.
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: polyfill pkg: preset-env PR: Dependency ⬆️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants