Part 4: Lazy load sound picker and image picker

[davidsbailey]

Background

See webpack content hashing design doc

Now that webpack is responsible for generating content hashes on js files, we can use lazy-loading to achieve code splitting.

Description

• Enables dynamic import syntax
• Introduces the loadable library for delay-loading react components
• lazy-loads SoundPicker and ImagePicker.

adhoc

This PR can be seen on this adhoc: https://adhoc-lazy-load-js-studio.cdn-code.org

performance

Because code-studio-common.js is included in every dashboard page, this change reduces the initial download size on every studio.code.org page by a whopping 1.7MB !

even though code-studio-common.js can be cached by the browser, this file can take several seconds for a browser to process on a slow computer. more detailed timeline analysis will be performed in one go, once a few other optimizations land.

screenshots

bundle analysis

before

baseline-content-hash.html.zip

after

lazy-load-sound-picker.html.zip

Future work

• there are several lazy-loading optimizations queued up behind this one.

[islemaster]

It looks like this is an older/unsupported version of this library. It's unclear to me what the difference between the old a new one is since they're pointing to the same GitHub repo. Did you have a particular reason for picking this one?

[davidsbailey]

Great feedback Brad!

The older version of loadable-components is because we aren't on Babel 7 yet.

[davidsbailey]

the next version after this one requires babel 7:
https://github.com/smooth-code/loadable-components/blob/v3.0.0/package.json#L17

[islemaster]

My first thought looking at this was that the Dialog import is going to be hoisted above the const declarations... but then I realized I have no idea if that's true anymore. So I'm curious exactly how @babel/plugin-syntax-dynamic-import works and if import hoisting isn't a thing we should assume anymore (not that it usually matters, but just in case).

The implementation file is tiny and just references into a config setting in Babel, so it's kind of funny that it's a plugin at all.

Looks like the dynamicImport option was originally added in babel/babylon#163 (the babylon project has since been pulled into the main babel/babel repo as the parser) and one of the key changes was in the statement parser. The latest version has abstracted this change into a secondary option allowImportExportEverywhere but I'm still not seeing where this is set.

I did some other digging in https://github.com/babel/babel and I can't find the actual hoisting behavior in the parser, so maybe I'll just run some local experiments on this.

[davidsbailey]

I was not familiar with scope hosting, but here's what I've found:

• https://webpack.js.org/configuration/mode states that module concatenation is enabled when webpack is run in production mode
• https://webpack.js.org/plugins/module-concatenation-plugin/ states that module concatenation implies scope hoisting
• https://webpack.js.org/plugins/module-concatenation-plugin/#optimization-bailouts lists a number of scenarios where it bails on scope hoisting. none of those mention the order of import statements within a file. they do, however, mention that an import() statement creates the "Root" of a new module, which is expected and in fact illustrated by the bundle analysis in this PR's description.

the Dialog import is going to be hoisted above the const declarations

my intuitive understanding of the word "hoist" is that it has more to do with pulling an entire module into the module that imports it, and does not have to do with moving statements around within a file. I have not found a definition which proves that, but I don't see anything in the module concatenation docs which mentions moving statements within files.

If you have a concern about correct behavior of this code please let me know a bit more about it?

[islemaster]

My question actually is about moving statements around, in particular about the ordering of side-effects in imported modules. I built a runnable example of import hoisting to help clarify my thoughts:

console.log('-- A --');
require('./thisFileLogsX');
console.log('-- B --');
import './thisFileLogsY';
console.log('-- C --');

Current behavior:

-- Y --
-- A --
-- X --
-- B --
-- C --

As Webpack is currently configured, imports are "hoisted" to the top of the module, the same way variable declarations are hoisted to the top of their scope in JavaScript. This means any side-effects in the files they import may run before code that appears to come before the import statement. On the other hand, require() runs when it is called, like any other code.

In practice this is rarely an issue because we try not to use modules with side-effects, but it comes up occasionally when interfacing with third-party libraries that depend on one another via globals (e.g. jQuery and its plugins).

I assume that dynamic import() calls do not get hoisted - they should behave like require() does now. My question is: Does this configuration change affect regular import statements too? Will it affect the behavior of this example?

[davidsbailey]

Interesting note on definitions -- the webpack docs call "module concatenation" and "scope hoisting" the same thing, and blog posts about webpack scope hoisting describe it as pulling content out of a child module up into the parent module which requires it. However, MDN docs and blog posts about javascript hoisting describe "hoisting" as moving certain statements to the top of the file. So, you used the term "hoisting" correctly and I mistook it for webpack's "scope hoisting".

The good news is I've run your example and I see the same order:

Dave-MBP:~/src/cdo/apps (lazy-load-js)$ grunt karma:entry --entry=./test/unit/hoist/hoistingTest.js 
...
PhantomJS 2.1.1 (Mac OS X 0.0.0) LOG: '-- Y --'
PhantomJS 2.1.1 (Mac OS X 0.0.0) LOG: '-- A --'
PhantomJS 2.1.1 (Mac OS X 0.0.0) LOG: '-- X --'
PhantomJS 2.1.1 (Mac OS X 0.0.0) LOG: '-- B --'
PhantomJS 2.1.1 (Mac OS X 0.0.0) LOG: '-- C --'
LOG: 'The test ran.'

[islemaster]

👍 LGTM then! It makes sense, given dynamic imports have a different syntax. Just wanted to make sure of no surprises here.

[islemaster]

Have you tested this in IE11? The docs for this plugin mention that IE requires polyfills for promise and iterator for this to work. I know we polyfill promise everywhere, I can't remember if we polyfill iterator.

[davidsbailey]

I had not, but I tested it just now and it seems to work fine in IE.

Our webpack config includes babel-polyfill, which includes core-js, which states that it includes polyfills for both promises and iterators.

I believe the warning you linked to applies to a configuration which we are not using, where babel tries to detect which polyfills are needed. Instead we always include all polyfills. this is explained here: https://babeljs.io/docs/en/6.26.3/babel-polyfill#usage-in-node-browserify-webpack

Those same docs also explain that we're using an unrecommended config:

We do not recommend that you import the whole polyfill directly: either try the useBuiltIns options or import only the polyfills you need manually (either from this package or somewhere else).

So we'll likely want to upgrade our configuration at some point and will need to deal with this issue at that time.

[davidsbailey]

Thanks for the thorough review, @islemaster ! Please let me know if you think any additional follow-up is needed at this time.

[islemaster]

👍 This is soooo awesome! Thank you for leading the charge on this work Dave. I'm really hoping that the workshop dashboard (already using react-router) and maybe the teacher dashboard see some benefit from this work in the near future too. Hooray! 🎉