Modify wp-scripts to transpile node_modules

[jameslnewell]

Description

I've modified wp-scripts to transpile modules imported from node_modules because a significant number of packages are transpiling down to the current JS standard instead of transpiling down to ES5. Importing these packages into an application built with wp-scripts breaks older browsers which are still supported by WordPress e.g. IE11.

This problem arises with many of sindresorhus' packages where he primarily targets NodeJS as encountered here.

Create React App and Parcel have already implemented similar solutions.

Aside: There appears to be a broader effort to solve the problems around dependencies and their targeted runtimes that doesn't affect bundle size like transpiling down to the lowest common denominator (ES5) does.

How has this been tested?

I created a new wp-scripts app, imported hex-rgb and checked the build output for let and const.

Screenshots

Types of changes

No external changes.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[jsnajdr]

Transpiling node_modules is a difficult topic with many edge cases and we (especially @blowery) spent some considerable effort in Calypso to make it right. See the discussion (that included one of CRA maintainers) in Automattic/wp-calypso#30196. I recommend adopting the approach that is used there.

Few areas where that could be improved are:

• unfortunately, one can't simply transpile everything in node_modules. Some packages (like Mapbox GL) break when transpiled twice and there needs to be a whitelist. See also Build: Transpile whitelisted packages in node_modules Automattic/wp-calypso#30196 (comment)
• CRA and Calypso also disable the transform-typeof-symbol transform, for reliability and performance reasons. See also Disable Symbol typeof transform facebook/create-react-app#5278 and Build: Transpile whitelisted packages in node_modules Automattic/wp-calypso#30196 (comment)

[jameslnewell]

unfortunately, one can't simply transpile everything in node_modules. Some packages (like Mapbox GL) break when transpiled twice and there needs to be a whitelist. See also Automattic/wp-calypso#30196 (comment)

How does CRA manage to avoid these issues without offering any configuration?

If we need to, how about an approach like:

• default to transpiling everything
• opt-in to transpiling specific modules via configuration e.g. something like:

package.json

{
  "wp-scripts": {
    "build": {
      "transpile-dependencies": [ "mapbox-gl" ]
    } 
  }
}

CRA and Calypso also disable the transform-typeof-symbol transform, for reliability and performance reasons. See also facebook/create-react-app#5278 and Automattic/wp-calypso#30196 (comment)

Can you please review my changes 🙏

[jsnajdr]

How does CRA manage to avoid these issues without offering any configuration?

That's a good question and I don't know the answer 🙂 Looking at CRA sources, it seems they transpile everything in node_modules without any blacklist or whitelist. Maybe @Timer knows more? Does CRA follow the approach that was recommended in Automattic/wp-calypso#30196 (comment) ?

[aduth]

Have you (or could you) measure the impact this would have on build time?

This concerns me for a few reasons, including:

• Wasted time for most modules not needing transpilation
• As already mentioned, potential for conflict in modules not expecting to be transformed this way

There's a lot to catch up on in some of the related discussions @jsnajdr mentions.

What I'd prefer most is:

• Having better awareness of where problems exist. Something like @tellthemachines's work in Add linting task to check IE compatibility #18774 should help here (I believe it is based on a similar approach in Calypso).
• Avoiding packages that make our lives difficult, when possible. My understanding of this is that it's typically encouraged that packages should target the lowest common denominator in their published versions (reference). Granted, I can see how this becomes more of a problem over time as the language evolves and what those target environments are (especially differing between Node and the browser).
• Opt for whitelisted transpilation when necessary.

[jameslnewell]

@aduth For a very simple app that I created purely for testing purposes, the increase in build time is <10% on both empty and primed caches. Are you able to recommend a larger repository that uses wp-scripts and the default wordpress.config.js for a more realistic test?

Inconclusive results from an overly simple app

Before:

first run:
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  5.27s user 1.15s system 82% cpu 7.749 total
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  5.23s user 1.14s system 80% cpu 7.921 total
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  4.83s user 1.02s system 96% cpu 6.059 total
- 5.11 avg

second run: 
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  2.11s user 0.50s system 49% cpu 5.261 total
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  1.82s user 0.41s system 56% cpu 3.965 total
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  1.75s user 0.40s system 51% cpu 4.152 total
- 1.89 avg

After:

first run:
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  5.58s user 1.17s system 75% cpu 8.894 total
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  5.36s user 1.08s system 93% cpu 6.858 total
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  5.30s user 1.05s system 92% cpu 6.851 total
- 5.41 avg - 6% increase

second run:
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  2.08s user 0.50s system 54% cpu 4.699 total
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  2.01s user 0.48s system 54% cpu 4.550 tota
./node_modules/@wordpress/scripts/bin/wp-scripts.js build  2.04s user 0.50s system 50% cpu 4.980 total
- 2.04 avg - 8% increase

[jsnajdr]

Maybe @blowery or @sgomes have some node-modules-transpilation build stats from Calypso? That's a project of a similar size to Gutenberg.

[blowery]

@jsnajdr I don't have any stats. We must transpile to keep using some libraries and support IE11 / ES5 browsers, so how much slower it makes the build isn't really relevant.

[sgomes]

That said, I would assume that transpiling everything would be a significant slowdown, and that's why we currently have an explicit whitelist system, right?

[blowery]

We've been pretty happy with a whitelist and a post-build validation step that verifies that we've caught everything that would fail a syntax check. First-party dependencies are a pretty easy to fix problem, it's the transitive stuff that will bite you. When you have a library that's ES5 but then updates to use debug@3, you then have to transpile debug to keep it working... As OSS projects drop IE11 support, you're going to see more and more breakage.

[blowery]

@sgomes It wasn't that slow from what I recall. The blocker was that it wasn't entirely safe.

[aduth]

Are you able to recommend a larger repository that uses wp-scripts and the default wordpress.config.js for a more realistic test?

To be honest, I thought we were using it in this project, which was my primary concern. I think a project the scale of Gutenberg (or in fact an intention to use this for Gutenberg) is a fair target, though.

[jameslnewell]

Regarding the impact on performance, I've copy/pasted the webpack rule into the main gutenberg wordpress.config.js file. Transpiling all node_modules saw an increase of 78% (46s to 82s) on an empty cache and an increase of 49% (10s to 15s) on a primed cache.

Before

Empty cache

./node_modules/.bin/wp-scripts build  46.56s user 2.69s system 317% cpu 15.525 total
./node_modules/.bin/wp-scripts build  46.11s user 2.63s system 314% cpu 15.499 total
./node_modules/.bin/wp-scripts build  46.56s user 2.69s system 315% cpu 15.597 total
avg=46.41

Primed cache

./node_modules/.bin/wp-scripts build  10.48s user 1.11s system 109% cpu 10.537 total
./node_modules/.bin/wp-scripts build  10.05s user 1.12s system 105% cpu 10.575 total
./node_modules/.bin/wp-scripts build  10.15s user 1.14s system 106% cpu 10.632 total
avg=10.23

After

Empty cache

./node_modules/.bin/wp-scripts build  81.77s user 5.78s system 471% cpu 18.572 total
./node_modules/.bin/wp-scripts build  85.50s user 5.95s system 462% cpu 19.759 total
./node_modules/.bin/wp-scripts build  80.73s user 5.78s system 459% cpu 18.816 total
avg=82.66 increase=78%

Primed cache

./node_modules/.bin/wp-scripts build  15.29s user 2.67s system 162% cpu 11.089 total
./node_modules/.bin/wp-scripts build  15.38s user 2.63s system 161% cpu 11.167 total
./node_modules/.bin/wp-scripts build  15.18s user 2.61s system 159% cpu 11.155 total
avg=15.28 increase=49%

Regarding the concern around modules not transpiling correctly, I've implemented the safelist approach proposed in #18798 (comment). Happy to adjust the approach as desired.

[gziolo]

@aduth and @jameslnewell – do we still want to have this change applied?

[aduth]

As far as I know, there's not a specific need for it at the moment, since hex-rgb was replaced in #18681. I have general concerns about the approach. If a need arises again, we can reevaluate the best option.