Multiple build targets #30768

sgomes · 2019-02-13T18:58:22Z

Implement support for multiple build targets in Calypso.

We currently have a single build, which includes everything needed to support the lowest denominator in features across supported browsers. This results in a large amount of polyfills and syntactic transformations which aren't needed for the majority of users (who thankfully do use modern browsers), and ends up bloating the bundle and increasing browser download and execution times.

This PR aims to generate a separate evergreen build in production, and serve that build instead to evergreen browsers, by doing user agent inspection. Other browsers will still work, since they have the fallback bundle, but known modern browsers will get the benefit of a smaller bundle.

There's also a small "mustard test" that tries to prevent an old browser that somehow got the modern bundle from breaking (which shouldn't happen unless it's reporting a modern browser's UA). It does this by trying to determine in runtime if the browser "cuts the mustard" by checking for support for ES modules (a reasonable test for browser "modernness" at this point in time). If it fails, the browser gets redirected to a forced fallback build with ?forceFallback=1. This is, as @blowery put it, a "belt and suspenders" approach, in trying to ensure that there is no breakage even if we somehow detect the UA incorrectly.

Note: This PR replaces #30420.

Changes proposed in this Pull Request

Remove duplicate browserslist definition in Babel config.
Use the browserslist definition to configure Terser ES6 and browser-specific settings.
Add second browserslist definition to package.json, for evergreen browsers.
Modify WebPack to output to different directories based on browserslist env.
Modify npm scripts to run two builds in production (one for fallback, one for evergreen).
Modify server-side code to read UA and choose a bundle accordingly.
Add "mustard test" to HTML sent to evergreen browsers.

Testing instructions

Evergreen:

Open in an evergreen browser (check browserslist definition for details of which browsers that means)
Check that page resources are coming from /calypso/evergreen/
Ensure that there is no breakage when using Calypso normally

Non-evergreen:

Open in a non-evergreen but supported browser (e.g. IE 11).
Check that page resources are coming from /calypso/fallback (not /calypso/evergreen/)
Ensure that there is no breakage when using Calypso normally

Compare the two:

Compare resource transfer sizes across both versions (particularly JS). The resources sent to the evergreen browser should be smaller and include ES6 syntax such as arrow functions (=>).

Forced fallback:

Open any page with ?forceFallback=1 in an evergreen browser
Check that page resources are coming from /calypso/fallback (not /calypso/evergreen/)
Ensure that there is no breakage when using Calypso normally

matticbot · 2019-02-13T18:58:23Z

Test live: https://calypso.live/?branch=try/multiple-build-targets

client/document/index.jsx

webpack.config.js

blowery · 2019-02-13T20:55:37Z

This currently breaks npm start for dev work. It only builds the fallback build and the server bits that generate URLs don't detect that the evergreen build is unavailable.

It would be nice to just build the evergreen build and use that for dev work.

sgomes · 2019-02-14T12:38:43Z

This currently breaks npm start for dev work. It only builds the fallback build and the server bits that generate URLs don't detect that the evergreen build is unavailable.

It would be nice to just build the evergreen build and use that for dev work.

Great catch! This is fixed. I removed the nomodule safeguard from dev builds and switched them to evergreen. Hot reloading should still work correctly.

sgomes · 2019-02-14T15:10:16Z

Looks like the desktop build is failing due to dependency issues. Does anyone have any idea what the issue might be, there?

The Safari 10 test also seems to be failing with a timeout. Unclear if that's related to the PR, but taking a look at the other open PRs it seems that it might be, since the other ones don't appear to be failing.

package.json

webpack.config.js

sgomes · 2019-02-15T10:39:37Z

Rebased and ran update-deps to fix merge issues in shrinkwrap file.

simison

From the SDK builder point of view, this is good now 👍

jsnajdr · 2019-02-15T12:45:25Z

One issue I found: the Gutenberg integration at /block-editor/post/:site/:post crashes when loading. The exception is swallowed in the async loader, but if you modify that code to report it, it turns out that regeneratorRuntime is missing.

The @babel/polyfill import no longer provides it, because the evergreen transpilation targets don't need it -- they natively support generators and async/await. But some Gutenberg packages (e.g., @wordpress/api-fetch) ship transpiled code that references the global. Filed an issue: WordPress/gutenberg#13890

sgomes · 2019-02-15T12:50:59Z

One issue I found: the Gutenberg integration at /block-editor/post/:site/:post crashes when loading. The exception is swallowed in the async loader, but if you modify that code to report it, it turns out that regeneratorRuntime is missing.

The @babel/polyfill import no longer provides it, because the evergreen transpilation targets don't need it -- they natively support generators and async/await. But some Gutenberg packages (e.g., @wordpress/api-fetch) ship transpiled code that references the global. Filed an issue: WordPress/gutenberg#13890

Thanks, @jsnajdr! Interesting issue. I wouldn't have expected the Gutenberg build to require something present that it isn't shipping with.

babel.config.js

client/document/index.jsx

package.json

webpack.config.js

sgomes · 2019-04-03T11:29:56Z

@sgomes Ran across this today: philipwalton/webpack-esnext-boilerplate#1 (comment)

Could you please check Edge and see what behavior it's exhibiting?

Yes, I'll double-check.

I just want to point out that the nomodule thing we're doing is just a sanity check over an abundance of caution; the main mechanism is actually UA-based. We could easily remove the safeguard if it's causing issues.

sgomes · 2019-04-03T11:49:30Z

@blowery

✅Edge 18 appears to be working correctly, loading resources a single time from the evergreen folder.

✅Edge 17 appears to be working correctly, loading resources a single time from the evergreen folder.

✅Edge 16 appears to be working correctly, loading resources a single time from the fallback folder.

❌Edge 15 has a hard fail ("This page is having a problem loading") loading calypso.live pages (not just the multiple builds branch), for some reason. It doesn't offer any useful errors.

✅IE 11 appears to be working correctly, loading resources a single time from the fallback folder.

blowery · 2019-04-03T18:10:55Z

@sgomes rockin! let's go!

blowery

LGTM

Thi PR reworks the build infrastructure to support two separate builds, and to serve the correct one to each user, based on user agent string. Squashed commits: Unify browserslist usage based on package.json. This commit: - Uses the browserslist definition to configure Terser ES6 and browser-specific settings. - Prepares package.json for multiple browserslists, to enable builds for multiple targets. - Defines the BROWSERSLIST_ENV environment variable in the NPM scripts, which is used by all browserslist-aware tools to pick the right configuration. Hardcode IE8 support to false Add documentation for chooseTerserEcmaVersion function. Make Calypso generate two builds: evergreen and fallback. We currently have a single, fallback build, which includes support for every supported browser. This results in a large amount of polyfills and syntactic transformations which bloat the bundle and increase browser download and execution times. This PR aims to generate a separate evergreen build in production, and serve that build instead to evergreen browsers, by doing user agent detection. Other browsers will still work, since they have the fallback bundle, but known modern browsers will get the benefit of a smaller bundle. Remove assignment from noModule property. Write fallback build files to public/fallback. Fix dev builds and make them use 'evergreen' settings. Rename isEvergreen prop to addEvergreenCheck Only run AssetsWriter for client builds. Ensure non-client builds use defaults instead of evergreen. Remove unreleased Safari versions from browserslist. Make fallback and evergreen builds happen in parallel. Fix arrow functions being used as constructors Ensure that BROWSERSLIST_ENV is always defined in WebPack Remove NODE_ARGS from build-client-both Ensure that `analyze-bundles` correctly uses the evergreen build. Ensure that icfy-analyze.js uses the evergreen build. Fix build cache paths after changes to the build process.

dmsnell · 2019-05-29T06:30:28Z

@sgomes after painful git bisecting I tracked this PR to the introduction of one of the two Critical dependency: the request of a dependency is an expression errors I'm seeing in the wp-desktop build

WARNING in ./calypso/node_modules/browserslist/node.js 116:26-33
Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
 @ ./calypso/node_modules/browserslist/index.js
 @ ./calypso/node_modules/browserslist-useragent/lib/index.js
 @ ./calypso/server/pages/index.js
 @ ./calypso/server/boot/index.js
 @ ./desktop/server/server.js
 @ ./desktop/server/index.js
 @ ./desktop/app.js
 @ ./desktop/index.js

WARNING in ./calypso/node_modules/browserslist/node.js 116:18-66
Critical dependency: the request of a dependency is an expression
 @ ./calypso/node_modules/browserslist/index.js
 @ ./calypso/node_modules/browserslist-useragent/lib/index.js
 @ ./calypso/server/pages/index.js
 @ ./calypso/server/boot/index.js
 @ ./desktop/server/server.js
 @ ./desktop/server/index.js
 @ ./desktop/app.js
 @ ./desktop/index.js

When I open the build from this commit I get a blank electron app showing the following error:

Error: ENOENT: no such file or directory, open 'calypso/server/bundler/assets-evergreen.json'
    at Object.fs.openSync (fs.js:584:18)
    at Object.module.(anonymous function) [as openSync] (ELECTRON_ASAR.js:173:20)
    at Object.fs.readFileSync (fs.js:491:33)
    at Object.fs.readFileSync (ELECTRON_ASAR.js:505:29)
    at q (/Volumes/Optane/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:319:2337)
    at Q (/Volumes/Optane/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:319:3119)
    at X (/Volumes/Optane/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/build/desktop.js:319:8954)
    at Layer.handle_error (/Volumes/Optane/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/node_modules/express/lib/router/layer.js:71:5)
    at trim_prefix (/Volumes/Optane/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/node_modules/express/lib/router/index.js:315:13)
    at /Volumes/Optane/wp-desktop/release/mac/WordPress.com.app/Contents/Resources/app/node_modules/express/lib/router/index.js:284:7

Any idea why we might be missing the assets file?

sirreal · 2019-05-29T07:49:53Z

Any idea why we might be missing the assets file?

I spotted a suspect line in wp-desktop and opened a PR to see if it helps: Automattic/wp-desktop#605

sgomes · 2019-05-29T10:05:51Z

Any idea why we might be missing the assets file?

This is likely because the build process is generating one thing (the fallback build) while the server-side runtime is expecting another (the evergreen one). Likely a bug I introduced when I switched which build Desktop uses 😞

Oh, actually I just saw @sirreal's PR. Not copying the file would do that too. Thanks, @sirreal !

sgomes · 2019-05-29T10:10:57Z

@sgomes after painful git bisecting I tracked this PR to the introduction of one of the two Critical dependency: the request of a dependency is an expression errors I'm seeing in the wp-desktop build

WARNING in ./calypso/node_modules/browserslist/node.js 116:26-33
Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
 @ ./calypso/node_modules/browserslist/index.js
 @ ./calypso/node_modules/browserslist-useragent/lib/index.js
 @ ./calypso/server/pages/index.js
 @ ./calypso/server/boot/index.js
 @ ./desktop/server/server.js
 @ ./desktop/server/index.js
 @ ./desktop/app.js
 @ ./desktop/index.js

WARNING in ./calypso/node_modules/browserslist/node.js 116:18-66
Critical dependency: the request of a dependency is an expression
 @ ./calypso/node_modules/browserslist/index.js
 @ ./calypso/node_modules/browserslist-useragent/lib/index.js
 @ ./calypso/server/pages/index.js
 @ ./calypso/server/boot/index.js
 @ ./desktop/server/server.js
 @ ./desktop/server/index.js
 @ ./desktop/app.js
 @ ./desktop/index.js

@dmsnell Hmm, these warnings are interesting. I have no idea what could be causing them, but they seem to becoming from inside the dependencies, right? Are they causing any failures?

sirreal · 2019-05-29T10:14:18Z

Related to the Critical dependency: the request of a dependency is an expression:
webpack/webpack#196

sirreal · 2019-05-29T10:19:40Z

Also: browserslist/browserslist#199

sgomes · 2019-05-29T10:24:42Z

Also: browserslist/browserslist#199

That's a much older version than what we're using. Ours should have the fix.

sgomes · 2019-05-29T10:26:49Z

In any case, the WebPack warning seems to be indicative of a potentially larger bundle, not any sort of failure. That's not an issue, since this is server-side code.

sirreal · 2019-05-29T10:40:19Z

The fix for browserslist/browserslist#199 seems to be adding browser property to package.json:

  "browser": {
    "./node.js": "./browser.js",
    "path": false
  },

source

According to the documentation (#, #), that would mean "node.js should be replaced by browser.js". That doesn't seem to be happening according to the stack trace on those warnings 😕

sgomes · 2019-05-29T10:43:00Z

It's not clear to me that's the right fix for us. That sounds like a fix for code that runs in a browser environment, whereas ours runs in a node environment.

dmsnell · 2019-05-29T21:21:25Z

In any case, the WebPack warning seems to be indicative of a potentially larger bundle, not any sort of failure. That's not an issue, since this is server-side code.

As far as I saw it didn't cause errors but I didn't check all the corners of the app. After going back and re-bisecting it looks like the iconv warning was there before this PR but the browserlist one is introduced here.

Thanks everyone for your efforts to clean this up. Since the desktop app is building again I think that I was partially mislead by the warnings. The assets are now working and the app runs at least…

sgomes added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build [Type] Performance labels Feb 13, 2019

sgomes requested a review from a team February 13, 2019 18:58

sgomes mentioned this pull request Feb 13, 2019

Unify browserslist usage based on package.json. #30420

Closed

sgomes changed the title ~~Try/multiple build targets~~ Try: multiple build targets Feb 13, 2019

blowery reviewed Feb 13, 2019

View reviewed changes

client/document/index.jsx Outdated Show resolved Hide resolved

blowery reviewed Feb 13, 2019

View reviewed changes

webpack.config.js Outdated Show resolved Hide resolved

sgomes force-pushed the try/multiple-build-targets branch from dadb6e7 to af2667f Compare February 14, 2019 13:02

sgomes force-pushed the try/multiple-build-targets branch from 96d010f to 2e25ad6 Compare February 14, 2019 18:18

simison reviewed Feb 15, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

simison reviewed Feb 15, 2019

View reviewed changes

webpack.config.js Outdated Show resolved Hide resolved

simison reviewed Feb 15, 2019

View reviewed changes

webpack.config.js Outdated Show resolved Hide resolved

simison requested review from a team February 15, 2019 08:14

sgomes force-pushed the try/multiple-build-targets branch from 8718bfb to 11dafec Compare February 15, 2019 10:39

simison approved these changes Feb 15, 2019

View reviewed changes

sgomes force-pushed the try/multiple-build-targets branch 3 times, most recently from 1ec4c3e to 98a88df Compare February 15, 2019 12:40

jsnajdr mentioned this pull request Feb 15, 2019

Published NPM package silently depend on the 'regeneratorRuntime' global WordPress/gutenberg#13890

Closed

jsnajdr reviewed Feb 15, 2019

View reviewed changes

babel.config.js Outdated Show resolved Hide resolved

client/document/index.jsx Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

webpack.config.js Show resolved Hide resolved

sgomes force-pushed the try/multiple-build-targets branch 2 times, most recently from 583170b to e2005dc Compare April 4, 2019 15:54

blowery approved these changes Apr 4, 2019

View reviewed changes

sgomes force-pushed the try/multiple-build-targets branch from e2005dc to bc63bc6 Compare April 5, 2019 11:14

sgomes force-pushed the try/multiple-build-targets branch from bc63bc6 to 1a743a1 Compare April 5, 2019 12:59

sgomes merged commit 03b3230 into master Apr 5, 2019

sgomes deleted the try/multiple-build-targets branch April 5, 2019 13:00

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

arunsathiya mentioned this pull request Apr 6, 2019

Editor: Facebook & Twitter Embeds Not Generating Preview #32088

Closed

sgomes mentioned this pull request Apr 6, 2019

Fix breakage in classic editor EmbedView. #32090

Merged

This was referenced May 20, 2019

Update desktop to use fallback builds #33173

Closed

Fix/desktop loader #33230

Merged

sirreal mentioned this pull request May 29, 2019

Build: Update assets filter Automattic/wp-desktop#605

Merged

sirreal mentioned this pull request Sep 20, 2019

SSR: Fix file-loader assets #36204

Merged

tyxla mentioned this pull request Feb 4, 2020

Remove yamlparser dependency #39242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple build targets #30768

Multiple build targets #30768

sgomes commented Feb 13, 2019 •

edited

Loading

matticbot commented Feb 13, 2019

blowery commented Feb 13, 2019

sgomes commented Feb 14, 2019 •

edited

Loading

sgomes commented Feb 14, 2019

sgomes commented Feb 15, 2019

simison left a comment

jsnajdr commented Feb 15, 2019 •

edited

Loading

sgomes commented Feb 15, 2019

sgomes commented Apr 3, 2019

sgomes commented Apr 3, 2019 •

edited

Loading

blowery commented Apr 3, 2019

blowery left a comment

dmsnell commented May 29, 2019

sirreal commented May 29, 2019

sgomes commented May 29, 2019 •

edited

Loading

sgomes commented May 29, 2019 •

edited

Loading

sirreal commented May 29, 2019

sirreal commented May 29, 2019

sgomes commented May 29, 2019

sgomes commented May 29, 2019

sirreal commented May 29, 2019 •

edited

Loading

sgomes commented May 29, 2019

dmsnell commented May 29, 2019

Multiple build targets #30768

Multiple build targets #30768

Conversation

sgomes commented Feb 13, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Feb 13, 2019

blowery commented Feb 13, 2019

sgomes commented Feb 14, 2019 • edited Loading

sgomes commented Feb 14, 2019

sgomes commented Feb 15, 2019

simison left a comment

Choose a reason for hiding this comment

jsnajdr commented Feb 15, 2019 • edited Loading

sgomes commented Feb 15, 2019

sgomes commented Apr 3, 2019

sgomes commented Apr 3, 2019 • edited Loading

blowery commented Apr 3, 2019

blowery left a comment

Choose a reason for hiding this comment

dmsnell commented May 29, 2019

sirreal commented May 29, 2019

sgomes commented May 29, 2019 • edited Loading

sgomes commented May 29, 2019 • edited Loading

sirreal commented May 29, 2019

sirreal commented May 29, 2019

sgomes commented May 29, 2019

sgomes commented May 29, 2019

sirreal commented May 29, 2019 • edited Loading

sgomes commented May 29, 2019

dmsnell commented May 29, 2019

sgomes commented Feb 13, 2019 •

edited

Loading

sgomes commented Feb 14, 2019 •

edited

Loading

jsnajdr commented Feb 15, 2019 •

edited

Loading

sgomes commented Apr 3, 2019 •

edited

Loading

sgomes commented May 29, 2019 •

edited

Loading

sgomes commented May 29, 2019 •

edited

Loading

sirreal commented May 29, 2019 •

edited

Loading