Build: Transpile whitelisted packages in node_modules #30196

blowery · 2019-01-16T22:25:29Z

In the past, we've tried very hard to avoid transpiling code in node_modules. This was because most code shipped in node_modules was plain ES5 that worked in all of our target browsers, so transpiling would be extra work with no benefit. Modules were typically ES5 because they wanted to support Node 4, which was on par with IE11 for language features.

Fast forward to today, where Node 4 is no longer supported. Some modules (debug, d3-*, striptags, ...) are dropping support for Node 4 and adding ES6 features to the code they ship to NPM, like let and for of. This breaks IE11, so we have to transpile these packages to use them in IE11.

This PR proposes to transpile some things in node_modules, provided by a whitelist. This hasn't seemed to affect total compilation time very much, especially for incremental runs when the babel cache is hot. We also use a reduced babel config that only transpiles the set of plugins supported by @babel/preset-env, reducing the surface area of features we'll transpile. We're not really expecting anything outside of the node@6 feature set, but using preset-env gives us a bit of future proofing.

The whitelist is path based, so it has no notion of a package's version. It would be a nice enhancement to only transpile packages that are past a certain version. debug@4, for instance, requires transpilation, but debug@3 does not. We may have both versions in the tree as transitive dependencies.

Original PR Description follows

In the past, we've tried very hard to avoid transpiling code in node_modules. This was because most code shipped in node_modules was plain ES5 that worked in all of our target browsers, so transpiling would be extra work with no benefit. Modules were typically ES5 because they wanted to support Node 4, which was on par with IE11 for language features.

Fast forward to today, were Node 4 is no longer supported. Some modules (debug, d3-*, striptags, ...) are dropping support for Node 4 and adding ES6 features to the code they ship to NPM, like let and for of. This breaks IE11, so we have to transpile these packages to use them in IE11.

This PR proposed to transpile most things in node_modules. It provides a blacklist of modules which should be skipped, but most things will run through a basic transpilation. This hasn't seemed to affect total compilation time very much, especially for incremental runs where the babel cache is hot. We also use a reduced babel config that only transpiles the set of plugins supported by @babel/preset-env, reducing the surface area of features we'll transpile. We're not really expecting anything outside of the node@6 feature set, but using preset-env gives us a bit of future proofing.

We may run into issues transpiling some packages, especially those that ship a minified version as the primary distribution. For those cases we provide a blacklist to exclude the package from transpilation.

A couple upgrades have slipped in and IE11 is broken on some screens (thankfully not login or home) when modules like d3-array@2 are used.

matticbot · 2019-01-16T22:25:30Z

Test live: https://calypso.live/?branch=fix/transpile-node-modules

jsnajdr · 2019-01-17T11:27:59Z

An idea how we could auto-test for untranspiled code: parse the production JS chunks with an ES5-only parser (does one exist? can existing popular parser to be configured to be ES5-only?) and verify that the parse succeeds.

blowery · 2019-01-17T15:25:48Z

An idea how we could auto-test for untranspiled code:

I think I'd rather just transpile it all if we can and avoid trying to detect what really needs it. Transpilation is pretty fast now and babel has a quality cache.

blowery · 2019-01-17T15:28:17Z

Looking at how CRA does it, they have a separate config for babel for dependencies. This is due to babel@7 now assuming that esm is in use. There's a config flag to make it autodetect like webpack that we should turn on.

jsnajdr · 2019-01-17T15:59:31Z

Interesting. TIL that CRA does that and that transpiling all node_modules is safe and reasonably fast.

blowery · 2019-01-18T15:28:17Z

@jsnajdr should work now?

blowery · 2019-01-18T15:29:58Z

Open questions:

Do we need to transpile for tests too? Tests run in node and all this fancy syntax in node_modules should run on Node 10, so ... probably not?
How much of a perf hit are we looking at?

jsnajdr · 2019-01-23T15:41:15Z

Do we need to transpile for tests too? Tests run in node and all this fancy syntax in node_modules should run on Node 10, so ... probably not?

We transpile tests with current Node as a target: https://github.com/Automattic/wp-calypso/blob/master/babel.config.js#L100-L107

Standard ES2018 should therefore transpile without any changes. The target is JSX and nonstandard extensions like class properties. Do any NPM modules ship that kind of syntax?

blowery · 2019-01-23T17:28:29Z

Do any NPM modules ship that kind of syntax?

Not that I've seen. This is mostly motivated by packages that are dropping old Node support and in the process picking up ES6 features, like let, for of, those sorts of things. I've not seen anyone shipping experimental syntax.

jsnajdr

Build speed doesn't seem to be significantly affected. It turns out that the modules from node_modules are in minority. There are 8520 files transpiled, only 3540 of them coming from node_modules.

I gathered all this data by placing a console.log statement to the babel-loader source.

babel.dependencies.config.js

jsnajdr · 2019-01-24T13:06:20Z

babel.dependencies.config.js

+		'@babel/react',
+	],
+	plugins: [
+		'@babel/plugin-syntax-dynamic-import',


This transform is also not needed. We don't expect import( ... ).then( ... ) statements in NPM packages, do we?

I don't think so, but CRA has it.... It would be odd.

To CRA it was added by @Timer in facebook/create-react-app#5047. The PR description doesn't mention that any published NPM package would use it.

It's a very minor issue that doesn't really matter at all. It's just my curiosity that's not 100% satisfied 😃

This transform is also not needed. We don't expect import( ... ).then( ... ) statements in NPM packages, do we?

We expect npm packages to use it in Create React App.

Also, compiling node_modules is not without its pitfalls, it's very unsafe.
You should avoid compiling node_modules if you can. 😄

though that does make me revisit the choice to use a blacklist. Maybe we need a whitelist instead.

I'd suggest keeping a list of "bad actors". If packages are intended to be used in the browser (even if they're Node packages) the publisher is responsible for compiling.
We always told users who tried to import non-ES5 that the publisher had to fix, and most of the time, they did.

Bad actors would contain often-used packages that refuse to compile themselves.

Insider tip: be sure to disable transform-typeof-symbol. This should fix mapbox and reduce some of the side effects of compiling node_modules if you go through with it.

@Timer I saw that in CRA@2 and wondered about it.. IE11 doesn't support Symbol at all, so dropping support for the typeof check seems ... chancy?

Ah, facebook/create-react-app#5278 has more background.

switch all this over to using a whitelist

webpack.config.js

jsnajdr · 2019-01-24T13:14:26Z

webpack.config.js

+				{
+					test: /\.jsx?$/,
+					include: /node_modules\//,
+					exclude: /@babel(?:\/|\\{1,2})runtime/,


There are several libraries that we can trust to be ES5:

babel-runtime -- the 6 version used still by many libraries: react-virtualized, click-outside, wpcom (!)

core-js

everything in @wordpress/*

lodash

regenerate-unicode-properties -- used by the @babel/plugin-proposal-unicode-property-regex transform

gridicons

fbjs

react and related

wpcom

One strange file that gets transpiled by Babel:

node_modules/css-loader/lib/css-base.js

I don't know why exactly, but it's probably the JS code that's added by css-loader and then removed again by the extract plugin?

Looks like exclude can be a function instead of a regex. Perhaps that would make building and maintaining that list easier. https://webpack.js.org/configuration/module/#condition

btw, react is not safe. They use Map and Set. :) https://reactjs.org/docs/javascript-environment-requirements.html

btw, react is not safe. They use Map and Set.

That is safe for us. We import @babel/polyfill at the very beginning, and it polyfills the Map and Set globals, according to the browser requirements in @babel/preset-env.

It wouldn't be safe if we:

used useBuiltins: 'usage' in preset-env. Babel needs to transpile a module to see that a polyfill is needed.

used corejs: true in transform-runtime. Then Babel transpiles usages of the Map global to imports from babel-runtime-corejs, avoiding the need for global namespace pollution.

In both cases, Babel would need to see the React module to polyfill correctly.

switched to a function based exclude in 40d46b8

also looked at trying to automate the set of includes with a function that parses each package.json and checks the engines property, but it looks like that won't fly because at least some of the packages that have dropped node 4 support have not updated engines.

See facebook/create-react-app#5278 for details. It makes all code slower and can break some libraries. Pulled out of #30196

ockham · 2019-02-21T12:56:42Z

I tend to agree with @Timer that it might make sense to err on the safe side when transpiling node_modules -- but I would love us to start doing it for those packages that actually publish their untranspiled code (so we can drop some of these aliases).

Some prior art here: #19698. FWIW, the heuristics I was using was based on the presence of a module or esnext field in package.json. Granted, the former only means ESM (and not necessarily ES6(+)), and the latter is more of a proposal at this point, but I thought there'd probably be a strong ESM-ES6(+) correlation (translating to 'untranspiled') anyway.

blowery · 2019-02-21T17:28:21Z

@ockham I agree, we'll want to use a whitelist, not transpile everything. I'm not in love with using module / esnext as a heuristic, as it means we'll likely be transpiling more than we need to, but it might serve as a check. Maybe we also come up with a new thing that scans the source and yells if it sees things that need transpilation?

See facebook/create-react-app#5278 for details. It makes all code slower and can break some libraries. Pulled out of #30196

* Disable babel transform-typeof-symbol See facebook/create-react-app#5278 for details. It makes all code slower and can break some libraries. Pulled out of #30196 * Move the exclude option to preset-env config

blowery · 2019-03-25T20:53:07Z

Switched this to use a whitelist in aea84cf

simison · 2019-03-26T09:26:11Z

Noting that Mapbox dependency might go away soon (~this week) in Calypso after migrating the block's code from Calypso to Jetpack.

In Calypso

wp-calypso/package.json

Line 143 in e486289

"mapbox-gl": "0.52.0",

In Jetpack
https://github.com/Automattic/jetpack/pull/11640/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R151

blowery · 2019-03-26T11:44:54Z

@simison with the new whitelist, we're no longer trying to transpile mapbox-gl.

jsnajdr

Posted two nits, looks good to 🚢

jsnajdr · 2019-03-28T13:47:38Z

webpack.config.js

@@ -160,6 +193,27 @@ function getWebpackConfig( {
 					cacheIdentifier,
 					exclude: /node_modules\//,
 				} ),
+				{
+					test: /\.jsx?$/,
+					include: _.memoize( shouldTranspileDependency ),


The memoize here is probably an insignificant optimization. It will maintain a map with several thousands entries, and webpack does so many checks on each filename so that this extra one is unlikely to make any impact.

jsnajdr · 2019-03-28T13:49:06Z

babel.dependencies.config.js

+		],
+	],
+	// Exclude transforms that make all code slower, see https://github.com/facebook/create-react-app/pull/5278
+	exclude: [ 'transform-typeof-symbol' ],


Does the exclude need to be there in two copies?

This would let us use many of the newer NPM modules that are dropping support for ES6 and requiring Node 4, 6, 10.

Teach that config to detect the module type. Teach webpack about the new config.

ockham · 2019-03-28T15:30:43Z

webpack.config.js

+							},
+						},
+					],
+				},


Could we use the Transpile shard from @automattic/calypso-build for this?

possibly, i'll try it out

jsnajdr · 2019-03-28T16:12:55Z

Thanks for fixing the nits! Looks good to me 👍

blowery added [Status] In Progress Build labels Jan 16, 2019

blowery self-assigned this Jan 18, 2019

blowery added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jan 18, 2019

Aljullu mentioned this pull request Jan 18, 2019

IE11 build requires checking new packages and adding to babel list woocommerce/woocommerce-admin#1176

Closed

jsnajdr reviewed Jan 24, 2019

View reviewed changes

blowery force-pushed the fix/transpile-node-modules branch from c225570 to 40d46b8 Compare January 25, 2019 19:45

blowery added a commit that referenced this pull request Jan 30, 2019

Disable babel transform-typeof-symbol

d427aa9

See facebook/create-react-app#5278 for details. It makes all code slower and can break some libraries. Pulled out of #30196

blowery mentioned this pull request Jan 30, 2019

Disable babel transform-typeof-symbol #30492

Merged

jsnajdr pushed a commit that referenced this pull request Feb 25, 2019

Disable babel transform-typeof-symbol

d927253

See facebook/create-react-app#5278 for details. It makes all code slower and can break some libraries. Pulled out of #30196

blowery mentioned this pull request Mar 6, 2019

Slideshow: IE11 Issues #31233

Merged

blowery force-pushed the fix/transpile-node-modules branch from b6e9f7b to 4166bbd Compare March 25, 2019 20:37

blowery requested a review from a team as a code owner March 25, 2019 20:37

blowery force-pushed the fix/transpile-node-modules branch from b1aa6f9 to aea84cf Compare March 25, 2019 20:49

blowery changed the title ~~Try to transpile node_modules~~ Build: Transpile whitelisted packages in node_modules Mar 25, 2019

simison mentioned this pull request Mar 26, 2019

fix(deps): update dependency debug to v4 #31407

Closed

1 task

blowery added the [Status] Needs e2e Testing label Mar 26, 2019

This was referenced Mar 26, 2019

fix(deps): update dependency striptags to v3 #31715

Closed

fix(deps): update dependency d3-scale to v3 #31523

Merged

blowery added [Status] Needs e2e Testing and removed [Status] Needs e2e Testing labels Mar 28, 2019

jsnajdr approved these changes Mar 28, 2019

View reviewed changes

blowery and others added 8 commits March 28, 2019 09:53

Try to transpile node_modules

4806603

This would let us use many of the newer NPM modules that are dropping support for ES6 and requiring Node 4, 6, 10.

Split off the babel config for things in node_modules.

6b55f4f

Teach that config to detect the module type. Teach webpack about the new config.

use a function to determine excludes

7007afa

exclude mapbox-gl from transpilation

fe12cb5

remove @babel/react

c40438b

Disable babel transform-typeof-symbol

e526c74

switch to whitelist, fix up excludes, use new babel/env settings

c9a998e

remove duplicate exclude

fcaf566

blowery force-pushed the fix/transpile-node-modules branch from d486c02 to fcaf566 Compare March 28, 2019 13:53

simison mentioned this pull request Mar 28, 2019

Build: Move babel.config.js to calypso-build package #31464

Merged

1 task

ockham reviewed Mar 28, 2019

View reviewed changes

blowery added 3 commits March 28, 2019 11:48

drop memoize of included modules

46465a2

switch to using the transpile shard

03aaf57

bump version on calypso-build

923433d

blowery merged commit 3388c69 into master Mar 28, 2019

blowery deleted the fix/transpile-node-modules branch March 28, 2019 18:33

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 28, 2019

jsnajdr mentioned this pull request Nov 28, 2019

Modify wp-scripts to transpile node_modules WordPress/gutenberg#18798

Closed

6 tasks

Build: Transpile whitelisted packages in node_modules #30196

Build: Transpile whitelisted packages in node_modules #30196

Conversation

blowery commented Jan 16, 2019 • edited Loading

matticbot commented Jan 16, 2019

jsnajdr commented Jan 17, 2019

blowery commented Jan 17, 2019

blowery commented Jan 17, 2019

jsnajdr commented Jan 17, 2019

blowery commented Jan 18, 2019

blowery commented Jan 18, 2019

jsnajdr commented Jan 23, 2019

blowery commented Jan 23, 2019

jsnajdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timer Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timer Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowery Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

ockham commented Feb 21, 2019

blowery commented Feb 21, 2019

blowery commented Mar 25, 2019

simison commented Mar 26, 2019

blowery commented Mar 26, 2019

jsnajdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr commented Mar 28, 2019

blowery commented Jan 16, 2019 •

edited

Loading

Timer Jan 28, 2019 •

edited

Loading

Timer Jan 29, 2019 •

edited

Loading

blowery Jan 28, 2019 •

edited

Loading