Published NPM package silently depend on the 'regeneratorRuntime' global

[jsnajdr]

Some published packages, e.g., @wordpress/api-fetch, use async/await internally and the published transpiled sources (in build or build-module) therefore depend on the regeneratorRuntime global.

When imported into an app that's transpiled to a target that supports async/await natively, there is suddenly no one who would provide the regeneratorRuntime global and the app starts crashing. Even if the app contains import '@babel/polyfill', the preset-env plugin won't include the regenerator runtime, because it's not needed by the transpilation target.

This started happening in Calypso recently:

• tests always run on the latest Node.js and therefore don't need to be transpiled much. Jest needs to be explicitly configured to include the regenerator runtime which wouldn't otherwise be there.
• we're experimenting with a custom build for modern evergreen browsers that ships ESnext code to the client. The Gutenberg integration crashes there because of missing regenerator runtime.
• the Desktop app is compiled with Electron target. I didn't check it yet, but the Gutenberg integration is also likely to crash there the same way.

Possible solution:
The root cause of the issue is that the published package depends on something that it doesn't declare as NPM dependency. The dependency is silent and implicit.

Using regenerator: true flag in the transform-runtime plugin would convert the global reference into an import statement:

const regeneratorRuntime = require( '@babel/runtime/regenerator' );
regeneratorRuntime.useIt();

Then the NPM package would have to declare a @babel/runtime dependency in package.json. That makes the existing dependency explicit rather than adding a new one.

Using a transpiled code in an environment that doesn't need it is still suboptimal, but at least not broken. Shipping untranspiled code in NPM packages is AFAIK still an active research area in the JS community rather that something we can use today 🙂

[gziolo]

Related PR where we changed how we transpile packages: #9643.

Just noting that @wordpress/api-fetch as well as all other Babel transpiled packages have @babel/runtime listed as a dependency:
https://github.com/WordPress/gutenberg/blob/master/packages/api-fetch/package.json#L24

[jsnajdr]

Related PR where we changed how we transpile packages: #9643.

Yes, I looked at that PR before filing, because I remember that some discussion about this already happened. When Gutenberg runs in the WordPress environment as a plugin, there is the wp-polyfill script being enqueued that provides the polyfills. That's not relevant in the NPM-published version though.

Babel transpiled packages have @babel/runtime listed as a dependency:

That's great, then we need to just enable the regenerator: true in the preset-env config. Currently Gutenberg uses the @babel/runtime only for helpers, i.e., the common functions used by non-trivial transpilations for classes, iterators etc.

[gziolo]

We also have this note left in all transpiled packages:

This package assumes that your code will run in an ES2015+ environment. If you're using an environment that has limited or no support for ES2015+ such as lower versions of IE then using core-js or @babel/polyfill will add support for these methods. Learn more about it in Babel docs.

Should it be expanded instead to include the note about regenerator? @aduth do you remember why regenerator was disabled? As far as I can see it's being defined inside wp-polyfill which is, in fact, part of babel-polyfill.

[jsnajdr]

The trouble is that @babel/polyfill can be a different thing in different environments. When transpiling for Electron or modern evergreen browsers, async/await and generators don't need to be polyfilled and therefore regeneratorRuntime is not present.

regeneratorRuntime is more similar to a Babel helper (like objectSpread or defineProperty) than to a polyfill.

Currently, published NPM packages import the helpers from @babel/runtime:

import _objectSpread from "@babel/runtime/helpers/esm/objectSpread";
import _objectWithoutProperties from "@babel/runtime/helpers/esm/objectWithoutProperties";

and they declare a dependency on @babel/runtime in package.json. They don't do a similar import from @babel/runtime/regenerator: they just reference the regeneratorRuntime global and expect it to exist.

The scripts intended to be loaded by WordPress, the ones in build/ that are built with webpack and assign to this.wp.* globals, each ship one copy of the helpers they use. And also depend on the regeneratorRuntime global.

If we enabled regenerator: true in Babel config, these scripts would each start bundling their own copy of the regenerator runtime, too. That's not entirely desired.

The best solution is therefore to declare regenerator: true for NPM packages: let's declare their dependencies programatically in package.json and in code rather than in README. And keep regenerator: false for the this.wp.* scripts. They will continue to use the global provided by wp-polyfill.

[sgomes]

One option I've seen some packages take is to include the untranspiled sources in the npm package (in addition to the regular transpiled code), and leave it to the consumer to use them at their own risk. It's not in any way the recommended usage, but it's there if you really want to do it, and there are no guarantees that it will never break should the project opt for a different Babel config.

I think that approach could work particularly well in our scenario, since the Calypso and Gutenberg teams work so closely together, and could communicate in anticipation of any eventual breaking changes. If anyone outside of Automattic wants to rely on the sources they could too, but again, it would come with no guarantees.

This approach would allow for a single, holistic transpilation, which would make it easier to avoid duplicated code and hard-to-find runtime bugs like this one.

[sgomes]

One option I've seen some packages take is to include the untranspiled sources in the npm package (in addition to the regular transpiled code), and leave it to the consumer to use them at their own risk. It's not in any way the recommended usage, but it's there if you really want to do it, and there are no guarantees that it will never break should the project opt for a different Babel config.

I think that approach could work particularly well in our scenario, since the Calypso and Gutenberg teams work so closely together, and could communicate in anticipation of any eventual breaking changes. If anyone outside of Automattic wants to rely on the sources they could too, but again, it would come with no guarantees.

This approach would allow for a single, holistic transpilation, which would make it easier to avoid duplicated code and hard-to-find runtime bugs like this one.

Actually, from https://unpkg.com/@wordpress/api-fetch@2.2.8/src/ this already appears to be the case.

@jsnajdr What do you think of Calypso switching to consume the sources instead?

[gziolo]

One option I've seen some packages take is to include the untranspiled sources in the npm package (in addition to the regular transpiled code), and leave it to the consumer to use them at their own risk. It's not in any way the recommended usage, but it's there if you really want to do it, and there are no guarantees that it will never break should the project opt for a different Babel config.

In the majority of cases, we add an entry point for React Native which is for the src folder.

"react-native": "src/index",

See also in: https://unpkg.com/@wordpress/api-fetch@2.2.8/package.json

If there is also a way to mark it explicitly in package.json file we can also include it to ensure can be considered deterministic :)

The best solution is therefore to declare regenerator: true for NPM packages: let's declare their dependencies programatically in package.json and in code rather than in README. And keep regenerator: false for the this.wp.* scripts. They will continue to use the global provided by wp-polyfill.

Let's explore it. I will discuss it with @aduth to ensure it doesn't conflict with anything done previously.

I can see regeneratorRuntime referenced as global in files published to npm, example:
https://unpkg.com/@wordpress/api-fetch@2.2.8/build/middlewares/fetch-all-middleware.js

[aduth]

Mostly reiterating things already mentioned, but for my own sake in organizing my thoughts:

• I think in Framework: Use babel-polyfill in place of runtime builtins #9643 there was a misunderstanding of what the regenerator: false option would even do, where if I had to guess, I'd have imagined it to have left generators / async functions unaltered. This is clearly not the case.
• Framework: Use babel-polyfill in place of runtime builtins #9643 was indeed primarily aimed at alleviating issues with how scripts are produced for core, where if possible, this should have been done as an override to the babel configuration used in the Webpack bundling process (mentioned already in previous comments).
• In a WordPress context, it's the wp-polyfill script which assigns the regenerator global (via @babel/polyfill)
  • I'm a bit curious about the previously mentioned point about @babel/polyfill not assigning the regenerator global in some environments, but it doesn't really impact the outcome.
• It's pretty clear there's an issue even as seen in the source snapshots, with references to an undefined global. I don't think it should be expected that regeneratorRuntime specifically is something assumed to be "polyfilled", where polyfill requirements as assumed as part of Framework: Use babel-polyfill in place of runtime builtins #9643 was features provided by newer language specifications.

  • gutenberg/packages/babel-preset-default/test/__snapshots__/index.js.snap

    Line 15 in d97251b

    regeneratorRuntime.mark(function _callee() {

So, I think the prior suggestion to only assume a regenerator as part of WordPress' specific build would be fine. I'd guess as far as actual changes, this includes removing the following lines from babel-preset-default/index.js:

gutenberg/packages/babel-preset-default/index.js

Line 23 in d97251b

corejs: false, // We polyfill so we don't need core-js.

gutenberg/packages/babel-preset-default/index.js

Line 25 in d97251b

regenerator: false, // We polyfill so we don't need regenerator.

(The first of these is confusing, but it's already the default setting)

And secondly, adding something in the root babel.config.js which would override the plugin configuration to restore regenerator: false ? Alternatively, if that's not possible or too cumbersome, I think there may be a separate solution using externals to assign '@babel/runtime/regenerator': 'regeneratorRuntime'.

I too am curious toward some of the points described in the original comment and I think illustrated by the misunderstanding described in my first point here about how specifically regenerator: false behaves, as far as creating builds for more capable browsers. I don't think this is something we need to solve now, but it might have an impact on how we phrase expectations around modules being for ES2015+ environments. What we will ship after this immediate fix will not assume an environment supporting newer syntaxes (generators, async/await), but will assume support for new methods (e.g. Object.assign, etc).

[gziolo]

Alternatively, if that's not possible or too cumbersome, I think there may be a separate solution using externals to assign '@babel/runtime/regenerator': 'regeneratorRuntime'.

I was thinking about the same approach the other day. I even tried something slightly similar in the past in #9275 but I'm sure making it external would be more accurate in this case.

[jsnajdr]

Alternatively, if that's not possible or too cumbersome, I think there may be a separate solution using externals to assign '@babel/runtime/regenerator': 'regeneratorRuntime'.

Yes, that should work very well :) @babel/transform-runtime will change the regeneratorRuntime global reference to a local identifier that is a binding to import from '@babel/runtime/regenerator'. It's Babel who performs this step. Then an external in webpack config will effectively undo this step, pointing the @babel/runtime/regenerator back to the regeneratorRuntime global.

I'm a bit curious about the previously mentioned point about @babel/polyfill not assigning the regenerator global in some environments

If the polyfill import

import '@babel/polyfill';

is left untransformed, it will always include the regenerator runtime, as well as the complete suite of JS polyfills from CoreJS.

It's the @babel/preset-env plugin that tries to optimize that. It replaces the import with a series of cherry-picked imports that include only the CoreJS subset that's needed by the target. And regenerator is included if and only if the transform-regenerator transform is active, i.e., if the target platform requires the transform. That's how the regenerator runtime can disappear.

corejs: false, // We polyfill so we don't need core-js.

This line should stay. If it was set to true it would transform references to globals that are affected by polyfills:

let x = Promise.resolve( 1 );

to

import _promise from '@babel/runtime-corejs2/core-js/promise';
let x = _promise.resolve( 1 );

That opens the door for a polyfill that doesn't pollute the global namespace. But CoreJS polyfills always do both: modify the globals and provide importable exports. We'd have to use the core-js-pure to avoid the global namespace pollution.

But we don't want to do anything like that. We ship code that uses the Promise global and assumes it exists. It's documented in the README as a (completely reasonable) requirement.

It's only the expectation that the regeneratorRuntime global exists that is not reasonable, as the global is not part of the web platform at all.

[aduth]

This line should stay.

It was mentioned for removal only because the default is false anyways, so as best I can tell there should be no impact in its removal?

[jsnajdr]

What do you think of Calypso switching to consume the sources instead?

@sgomes That would be optimal as it potentially produces the most optimized code. But I don't like the following quote at all 😄

since the Calypso and Gutenberg teams work so closely together, and could communicate in anticipation of any eventual breaking changes. If anyone outside of Automattic wants to rely on the sources they could too, but again, it would come with no guarantees.

The published @wordpress/* packages should be equally useful and accessible for everyone, no matter if they are inside or outside Automattic. It should be always clear what syntax these sources use and how to transpile and use them correctly.

Is there really something that qualifies as an "undocumented API" that's comprehensible only to insiders? I only know about the slightly unusual way to transpile JSX (with @wordpress/element) and that's all.

[jsnajdr]

It was mentioned for removal only because the default is false anyways

Oh yes, I misunderstood that. What I really wanted to say is "this value (false) should stay" 🙂 The default is indeed false.

[sgomes]

Is there really something that qualifies as an "undocumented API" that's comprehensible only to insiders? I only know about the slightly unusual way to transpile JSX (with @wordpress/element) and that's all.

It's not so much an undocumented API as an implicit one, in the form of Babel configurations; what JS features (and potentially what version of them) we're enabling. For example, class field declarations, private fields, etc. If at some point Gutenberg decides to add support for a newer syntax feature and make use of it in its code, every consumer of its sources will have to add that support too.

I was suggesting that it would be easier to coordinate such changes inside Automattic, so as to avoid breakage.

Otherwise, if Gutenberg would like to treat all clients equally and support building from source, then it would have to start treating its sources as an interface unto itself and view them as an officially supported product, with all the versioning requirements that would entail. Adding support for a new JS feature would be a major version change, for example.

[aduth]

Related pull request: #14130

[aduth]

Discussed in Slack yesterday with @swissspidy (link requires registration):

https://wordpress.slack.com/archives/C02QB2JS7/p1556142662462000

While this is fixed in the code, some packages still need to be published for it to be reflected (@wordpress/token-list was mentioned in the thread). This is likely due to the fact that the fix in #14130 did not touch each of the individual packages, so as far as Lerna is concerned, there was no need to publish new versions (and the packages have otherwise received no other updates).

Remaining task: Audit and publish packages which would have been impacted by this fix and have not been published since it was merged.

[gziolo]

I double checked the latest version of @wordpress/token-list:
https://unpkg.com/@wordpress/token-list@1.3.0/build/index.js

and it looks good.

I also enforced publishing all packages last time I did major/minor version bump.