Feature: Add Code Splitting

[cfilby]

Issue: Initial implementation for Issue #588 to add Code Splitting to reduce overall bundle size.

Notable changes in this PR:

• Adds React-Loadable
• TS Config has been changed to target esnext - this is required to support proper use of import with code splitting. Link. In my own experimentation this change was required - without it code splitting wasn't happening. moduleResolution node is also required to support the existing barrel exports.
• Remove bulk of route tests. All Page components are wrapped in Loadable which prevents direct comparison of eventually rendered component.
• Overall main bundle size has dropped. On master the main js bundle with mage watch is 52.8kb, which jumped to 63.4kb with esnext enabled, dropping to 37kb for the main bundle after the router was refactored to use react loadable.

Comments/Future Work:

• At the moment the use of barrel exports/imports seems like it's hamstringing the code splitting. We could probably further reduce the main bundle size by auditing the barrel imports in components and pages.
• Currently getting some CSS Code Splitting warnings that should be probably reviewed
• Currently getting some jest test warnings about esModuleInterop. Might be worth enabling esmoduleinterop in the typescript config and fixing the current import styles from import * as X from 'y' to import X from 'y'.

@goenning Did you have any thoughts on the current code splitting implementation? I figured I'd check in with an initial implementation of the code splitting and see what direction we should take it from here. I'd be happy to open another PR if we wanted to tweak the current import style/further prune down the bundle size.

[goenning]

At the moment the use of barrel exports/imports seems like it's hamstringing the code splitting. We could probably further reduce the main bundle size by auditing the barrel imports in components and pages.

How does barrel imports impacts that? I though webpack would only pick what we have imported. The size of main.js is much better now, but if we can reduce even further that'd be great.

The main.js bundle contains all the components inside components/common folder. These components change very rarely. So moving them into a common.js bundle would reduce the size of main.js as well. (this could be a completely isolated PR, what do you think?)

Currently getting some CSS Code Splitting warnings that should be probably reviewed

Do you know why is that happening? I wonder if it's related to how we import the scss files.

Currently getting some jest test warnings about esModuleInterop. Might be worth enabling esmoduleinterop in the typescript config and fixing the current import styles from import * as X from 'y' to import X from 'y'.

Not much to comment here, I'll read about esModuleInterop.

Remove bulk of route tests. All Page components are wrapped in Loadable which prevents direct comparison of eventually rendered component.

I like this tests and I think they are important. I have a suggestion here. If we change route to:

const route = (path: string, importPathSuffix: string, componentName: string, showHeader: boolean = true): PageConfiguration => {
  path = path
    .replace("/", "/")
    .replace(":number", "\\d+")
    .replace(":string", ".+")
    .replace("*", "/?.*");

  const regex = new RegExp(`^${path}$`);
  const component: LoadablePage(importPathSuffix, componentName);
  return { regex, component, componentName, showHeader };
};

We can then change the tests to check for the componentName property instead of the component class.

@goenning Did you have any thoughts on the current code splitting implementation? I figured I'd check in with an initial implementation of the code splitting and see what direction we should take it from here. I'd be happy to open another PR if we wanted to tweak the current import style/further prune down the bundle size.

It's a great start! I actually have more questions than thoughts. For example:

• It's generating 60+ tiny JS and CSS files, but most of them are not being used by the application. Is this due to how we import css? I was expecting one chunk for each Page component.
• Why is js/45.0625e287ea2b04c4dddd.js so big? It's 485 KiB. That's even more than vendor+main combined on a master branch. I even opened this file, but didn't understand where did it came from.
• For some reason, the browser is not caching the chunks, even when the server is returning Cache-Control headers.
• The chunks are not loading from the CDN (when configured). I saw that we can enable this with __webpack_public_path__. We'll also need __webpack_nonce__ because we have a strict CSP enabled.

I prefer multiple small PR rather than a big one. So let's try to keep this one reasonable size and then move on to the next.

Sorry for the big reply and thanks for the PR 😄

[codecov-io]

Codecov Report

Merging #596 into master will decrease coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
- Coverage   74.92%   74.81%   -0.11%     
==========================================
  Files         100      100              
  Lines        6423     6426       +3     
==========================================
- Hits         4812     4807       -5     
- Misses       1260     1272      +12     
+ Partials      351      347       -4

Impacted Files	Coverage Δ
app/pkg/email/smtp/smtp.go	16.03% <0%> (-2.43%)	⬇️
app/handlers/oauth.go	62.79% <0%> (-0.85%)	⬇️
app/cmd/routes.go	87.18% <0%> (ø)	⬆️
app/pkg/web/context.go	84.06% <0%> (+0.13%)	⬆️
app/pkg/web/util/webutil.go	96.15% <0%> (+0.15%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e63c7de...7c56b53. Read the comment docs.

[cfilby]

Ah! So it seems like the main issue was the use of the function to dynamically generate a loadable page/imports. My guess is that since the resolve path/component wasn't static Webpack was having a hard time splitting the code into chunks, leading to the creation of 63 chunks. Once I created a statically defined set of AsyncPages I'm now getting 17 chunks (which corresponds to one per page). That said, I haven't played with webpack directly too much, so it's possible I'm still missing something or drawing incorrect conclusions.

I still think some caution will be required when dealing with the barrel imports, at least in the non-dynamically imported areas of the application. For example you can break the current code splitting by changing the ErrorBoundary import ErrorPage statement to use @fider/pages (since that import imports every single page). I'd have to investigate whether or not the barrel imports have any size impacts in dynamically loaded components, but the conclusion I drew earlier is probably wrong given that it was based on my initial bad implementation of code splitting.

To address the other questions:

• I'd be good with making a separate PR for the common components - makes sense to me.

• Route tests re-added - now check against the async pages

• CSS warnings should be resolved. Moved the css imports for the Posts/TagsFilters to the top of the page.

• esModuleInterop warnings are still around:

message TS151001: If you have issues related to imports, you should consider setting `esModuleInterop` to `true` in your TypeScript configuration file (usually `tsconfig.json`). See https://blogs.msdn.microsoft.com/typescript/2018/01/31/announcing-typescript-2-7/#easier-ecmascript-module-interoperability for more information.

• I think the caching issue might now be addressed, but I'm not certain. At least when running mage watch on subsequent reloads I'm seeing from memory/disk cache on the chunks (although I'm not sure if the caching settings differ in dev vs prod).

• I can look into the Webpack CDN stuff for this PR, or I could make a separate PR if you'd prefer that be handled separately.

Let me know what you think! Sorry about the snafu with the first go around!

[goenning]

Great progress, It's looking better now! I have updated #588 with what I believe we can have as separate PR's. Have a look and let me know what you think.

For this PR:

• move react-loadable to vendor bundle (if possible);
• use [chunkhash] on MiniCssExtractPlugin. I don't know why I used [hash], which basically changes son every build even if the CSS doesn't. I can't believe I had this configuration since the beginning 😢
• CDN is required, let's implement on this PR;
• we need a better design for Loading component. We could probably use the Loader component, which has a built-in delay already;

Thank you!

[cfilby]

Sounds good! I'll look into these tonight - thanks again!

[cfilby]

How's this look? Thanks again!

[goenning]

I need to give it a try and test it. I'm not sure if this CDN setup will work. The reason being that both nonce and the assets base URL are not static nor known during webpack build time. Both values are only available during runtime. If I'm not mistaken, the assetsURL is available on window and by consequence, on Fider object as well. For the nonce ID, we use the ContextID which is probably not exposed to JavaScript and changes on every request. I guess we should expose so that we can set it to __webpack_nonce__.

I'll be offline until tuesday/wednesday, so my responses will be delayed. Sorry about that and thanks for the update 😄

[goenning]

After merging esInterop, the CI run didn't code split at all. By the way, have you seen that React 16.6 has been released with a native option for code splitting? We could give that a try and remove the extra react-loadable dependency. What do you think?

As for the CDN, if you merge master into your branch, you'll be able to use two new properties:

• Fider.session.contextID: this is the nonce we need to set on webpack;
• Fider.settings.globalAssetsURL: this is the system-wide base CDN URL for assets;

[cfilby]

Hmm, I'm wondering if esInterop breaks code splitting (not entirely sure what all it does under the hood to create the default imports) - haven't really found consistent documentation to that effect stating it one way or another though. I'll try to spend some more time doing more research on it.

I'd definitely agree with using the language built in lazy loader if it works without issue - no sense adding another dependency to the project!

And awesome! I'll update the webpack vars accordingly in the index.tsx file.

[cfilby]

Code splitting is working again (goofed in the esInterop PR and didn't remove the use of @fider/pages to import the error page in ErrorBoundary) and I've tried adding the globalAssetPath/contextID from Fider.

The React types don't yet include type information for Suspense/Lazy - did you still want to include those now with some ignore flags or just wait until the next types/react release and migrate to Suspense/Lazy then?

[goenning]

Awesome! I had to tweak a bit the public_path variable to make the CDN work.

I left a few things we need to change and then we can merge this.

Let's keep the Suspense/Lazy for another PR. I just saw the docs and the component needs to be exported as default, which we don't usually do on Fider. So that'll require a slightly bigger change and simply replacing react-loadable with lazy. This PR is already extremely useful, so let's get this into master 😄

[cfilby]

Alright, I believe I have those changes integrated. Let me know if there's anything else you want me to change!

Thanks again for all your patience, especially with the webpack nonce and asset path!

[goenning]

It's merged 🎉

Thank you for another great PR!