Framework: Implement AsyncLoad for code-splitting using Babel plugin #8544

aduth · 2016-10-06T14:56:06Z

Supersedes #5356

This pull request is related to #5356, implementing <AsyncLoad />, but leveraging a new Babel plugin transform-wpcalypso-async to handle transformations of prop values under the hood to require.ensure or require depending on the current environment configuration.

The new transform plugin also exposes a lower-level asyncRequire function for general usage. See plugin documentation for more information.

Testing Instructions:

Navigate to the editor, verifying that the sharing accordion is displayed after visiting the page.

Navigate to the post editor
Select a site, if prompted
Note that a Sharing accordion appears in the sidebar

Follow-up Tasks:

We should improve <AsyncLoad /> to accept additional props to control when the module is required, so that we can load in response to specific interactions (e.g. hovering the accordion title).

cc @mtias @ehg @mcsf @seear @umurkontaci

matticbot · 2016-10-06T14:56:08Z

Test live: https://calypso.live/?branch=try/async-load-babel-plugin

blowery · 2016-10-06T15:00:15Z

webpack.config.js

@@ -112,7 +112,14 @@ if ( CALYPSO_ENV === 'desktop' || CALYPSO_ENV === 'desktop-mac-app-store' ) {
 jsLoader = {
 	test: /\.jsx?$/,
 	exclude: /node_modules/,
-	loaders: [ 'babel-loader?cacheDirectory' ]
+	loader: 'babel',
+	query: {


probably want to turn cacheDirectory back on?

probably want to turn cacheDirectory back on?

Yep, good catch. Fixed in baab477a9e1a6a1e9c5ef2872ea897dd63c297d2

aduth · 2016-10-06T15:07:32Z

A visualization of the transformations applied by the plugin:

With code splitting:

Without code splitting:

ehg · 2016-10-07T10:53:13Z

webpack.config.node.js

+				loader: 'babel',
+				query: {
+					plugins: [
+						[ 'transform-wpcalypso-async', {


Do we want this enabled for the server bundle?

Do we want this enabled for the server bundle?

I think so? Otherwise AsyncLoad and asyncRequire won't be transformed.

We don't want multiple bundles being generated though, in any case, so can we set async to false?

We don't want multiple bundles being generated though, in any case, so can we set async to false?

Yeah, that makes sense.

blowery · 2016-10-07T20:27:22Z

client/post-editor/editor-drawer/index.jsx

@@ -12,7 +12,7 @@ import Accordion from 'components/accordion';
 import AccordionSection from 'components/accordion/section';
 import Gridicon from 'components/gridicon';
 import CategoriesTagsAccordion from 'post-editor/editor-categories-tags/accordion';
-import EditorSharingAccordion from 'post-editor/editor-sharing/accordion';
+import EditorSharingAccordion from 'async-component!post-editor/editor-sharing/accordion';


how does this work with tests?

how does this work with tests?

Probably not very well, and I'm sure same goes for base <AsyncLoad />. Will need to think about this some more.

Maybe the loader could check for the NODE_ENV and return the original source if it's testing?

Hmm, I guess the issue is that Webpack isn't used at all in the context of tests, so it'll choke on the loader syntax...

I would probably split the async-component! loader syntax up into another PR, since IMHO it adds a fair bit of complexity.

I would probably split the async-component! loader syntax up into another PR, since IMHO it adds a fair bit of complexity.

What would you suggest be included in this pull request after that would be removed?

Anything needed for asyncRequire and AsyncLoad. That way, I think we can split up changes to the build pipeline into more discrete pieces.

Anything needed for asyncRequire and AsyncLoad. That way, I think we can split up changes to the build pipeline into more discrete pieces.

So, to be clear, for this instance we'd use <AsyncLoad /> directly instead of the loader in this pull request, and defer the loader to a future one?

Just want to be sure we're including something in this one. 😆

So, to be clear, for this instance we'd use directly instead of the loader in this pull request, and defer the loader to a future one?

Sorry, yes. I see this PR as being for usage of AsyncLoad and asyncRequire?, along with the babel plugin required to transform those. Then the import webpack loader being in another PR, so we're not changing too much at once :)

aduth · 2016-10-07T20:29:46Z

Couple issues to resolve:

Currently with the Webpack loader it's emitting a chunk with the full absolute path of the module
I think I'll need to update the Babel plugin to pass the async chunk name with path separators replaced

aduth · 2016-10-10T15:30:25Z

I found that you can reference Babel plugins via local file paths, so I decided to bring the plugin inside the project (at least for faster iterations in this branch).
My hacky workaround for the Mocha tests issue is to enhance the Babel plugin to strip the loader syntax from the import path (e279107), at which point Mocha will treat it as a synchronous require. I think a long-term solution would be to run modules through Webpack before passing them to Mocha, which would be more aligned with our generated bundles.

ehg

Oops, forgot to submit.

ehg · 2016-10-18T16:26:44Z

webpack.config.js

@@ -156,6 +164,6 @@ if ( config.isEnabled( 'webpack/persistent-caching' ) ) {
 	webpackConfig.plugins.unshift( new HardSourceWebpackPlugin( { cacheDirectory: path.join( __dirname, '.webpack-cache', 'client' ) } ) );
 }

-webpackConfig.module.loaders = [ jsLoader ].concat( webpackConfig.module.loaders );
+webpackConfig.module.postLoaders = [ jsLoader ];


Having the babel transformation happen in a postLoader worries me a little. Was this just for the import syntax change?

Having the babel transformation happen in a postLoader worries me a little. Was this just for the import syntax change?

Yes, if I recall correctly, it was so that:

(a) The async component loader could return ES6
(b) The Babel plugin would be run last to strip the async-component! import prefix

ehg · 2016-10-18T16:31:17Z

client/post-editor/editor-drawer/index.jsx

@@ -12,7 +12,7 @@ import Accordion from 'components/accordion';
 import AccordionSection from 'components/accordion/section';
 import Gridicon from 'components/gridicon';
 import CategoriesTagsAccordion from 'post-editor/editor-categories-tags/accordion';
-import EditorSharingAccordion from 'post-editor/editor-sharing/accordion';
+import EditorSharingAccordion from 'async-component!post-editor/editor-sharing/accordion';


I would probably split the async-component! loader syntax up into another PR, since IMHO it adds a fair bit of complexity.

aduth · 2016-10-26T16:39:18Z

Ok, I performed a surgical rebase, squashed a few commits, but preserved the Webpack loader and a revert so it's kept in history for future reference. How does this look now @ehg?

blowery · 2016-10-26T18:34:00Z

client/components/async-load/index.jsx

+		};
+	}
+
+	componentWillMount() {


this may also need to have componentWillReceiveProps in case we get into a situation where React decides to reuse an instance instead of spinning up a new one. Given how this will be used, it's probably unlikely, but worth doing.

It also raises the point that the Babel plugin transforms the string into a function which is created anew for every render. I might experiment a bit to see how difficult it would be to pull the function to the top of the transformed module so that we can do a strict equality check in componentWillReceiveProps before calling the require prop.

This brings to mind another Babel plugin that might be interesting to experiment with:

http://babeljs.io/docs/plugins/transform-react-constant-elements/

~~that thing would be amazing for gridicons~~

er, no, they take a dynamic size. darn.

blowery · 2016-10-26T18:35:04Z

server/bundler/babel/babel-plugin-transform-wpcalypso-async/index.js

+				const isAsync = state.opts.async;
+
+				// In both asynchronous and synchronous case, we'll finish by
+				// calling reqquire on the loaded module


speling on require

speling on require

Fixed in rebased 5b9a658

ehg · 2016-10-27T11:38:20Z

Ok, I performed a surgical rebase, squashed a few commits, but preserved the Webpack loader and a revert so it's kept in history for future reference. How does this look now @ehg?

It looks good. Thanks for this, I hope not too much anesthesia was required :)

blowery · 2016-10-27T12:07:47Z

Bit late but something to maybe consider?

https://github.com/airbnb/babel-plugin-dynamic-import-webpack

aduth · 2016-10-27T19:42:38Z

Bit late but something to maybe consider?

It's not obvious the point of this plugin vs. simply using require.ensure... to turn it into a Promise I guess? In our case we needed to handle transforming to either require.ensure or require based on the environment before allowing it to be processed by Webpack.

blowery · 2016-10-28T18:07:47Z

@aduth yeah, true. I'm happy with what we have anyway. :)

aduth · 2016-10-28T18:46:20Z

Introduced function hoisting for the transformed require prop in c26d2c9 for use by componentDidUpdate lifecycle method of <AsyncLoad /> component in 7640e48.

aduth · 2016-10-28T18:52:20Z

Updated in/out with { "async": true }:

ehg · 2016-11-01T17:18:06Z

The hoisting changes look good. I think we can merge this now! Great work @aduth.

mtias · 2016-11-01T18:12:54Z

Thanks for working on this!

This reverts commit 1e5715ba34b9620dea5b90b9b2d8766973fde9df.

…nents

aduth · 2016-11-02T18:32:28Z

Did a few more final rounds of testing (production mode, IE, desktop app). Looking good on all fronts. Going to merge.

aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components labels Oct 6, 2016

blowery reviewed Oct 6, 2016

View reviewed changes

ehg reviewed Oct 7, 2016

View reviewed changes

aduth force-pushed the try/async-load-babel-plugin branch from baab477 to 053fb7a Compare October 7, 2016 14:33

blowery reviewed Oct 7, 2016

View reviewed changes

ehg requested changes Oct 20, 2016

View reviewed changes

aduth force-pushed the try/async-load-babel-plugin branch from e279107 to c6757d9 Compare October 26, 2016 16:38

blowery reviewed Oct 26, 2016

View reviewed changes

ehg approved these changes Oct 27, 2016

View reviewed changes

aduth mentioned this pull request Oct 28, 2016

Framework: Introduce AsyncLoad for asynchronous rendering by code splitting #5356

Closed

aduth force-pushed the try/async-load-babel-plugin branch from c6757d9 to c26d2c9 Compare October 28, 2016 18:38

ehg mentioned this pull request Nov 1, 2016

Framework: Improve CSS management #8957

Closed

ehg added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 1, 2016

aduth added 7 commits November 2, 2016 13:40

Framework: Incorporate transform-wpcalypso-async Babel plugin

6e1af40

Components: Add AsyncLoad component for bundle loading

2324b0e

Editor: Load sharing accordion asynchronously

e73a8b7

Framework: Add async-component Webpack loader

dfa58f0

Revert "Framework: Add async-component Webpack loader"

524f714

This reverts commit 1e5715ba34b9620dea5b90b9b2d8766973fde9df.

Framework: Hoist transformed function expression from AsyncLoad compo…

87318f1

…nents

Components: Trigger require if prop reference changes

808cbb6

aduth force-pushed the try/async-load-babel-plugin branch from 7640e48 to 808cbb6 Compare November 2, 2016 17:45

aduth merged commit 430231f into master Nov 2, 2016

aduth deleted the try/async-load-babel-plugin branch November 2, 2016 18:32

aduth mentioned this pull request Nov 7, 2016

Framework: Pass default export in asyncRequire callback #9190

Merged

lancewillett removed the [Status] Ready to Merge label Nov 23, 2016

aduth mentioned this pull request Mar 3, 2017

Extensions: Add extension reducers to static build #11728

Merged

Framework: Implement AsyncLoad for code-splitting using Babel plugin #8544

Framework: Implement AsyncLoad for code-splitting using Babel plugin #8544

Conversation

aduth commented Oct 6, 2016

matticbot commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Oct 6, 2016 • 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

aduth Oct 21, 2016 • 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

aduth commented Oct 7, 2016 • edited Loading

aduth commented Oct 10, 2016

ehg 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

aduth commented Oct 26, 2016

Choose a reason for hiding this comment

aduth Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowery Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehg commented Oct 27, 2016

blowery commented Oct 27, 2016 • edited Loading

aduth commented Oct 27, 2016

blowery commented Oct 28, 2016

aduth commented Oct 28, 2016

aduth commented Oct 28, 2016

ehg commented Nov 1, 2016

mtias commented Nov 1, 2016

aduth commented Nov 2, 2016

aduth commented Oct 6, 2016 •

edited

Loading

aduth Oct 21, 2016 •

edited

Loading

aduth commented Oct 7, 2016 •

edited

Loading

aduth Oct 26, 2016 •

edited

Loading

blowery Oct 26, 2016 •

edited

Loading

blowery commented Oct 27, 2016 •

edited

Loading