Lazy load routes and some components

[sorin-davidoi]

Reduces application.js by ~30% (from 648kB to 455kB). There are still some things to address like error handling and perhaps show the loading indicator while fetching the bundles.

http://bl.ocks.org/sorin-davidoi/raw/bcce9c6bb4ae822ea8f26f4a5f6eae60/

[akihikodaki]

Why not just initialize staticColumns with null and test it is null? I guess you did so in favor of const, but const does not really make difference in this case because it just eliminates a possibility (reassignment) to get broken while there are many other similar possibilities.

[sorin-davidoi]

Because if I do that I am basically waiting for all three columns to be fetched until I display them. This way as soon as a column is fetched it is displayed. Also, if fetching just one of them fails, the user is still able to interact with the other two.

[nolanlawson]

This is really neat. I need to test this, but some quick thoughts:

• errors could be communicated via generic toasts; I believe this is what the emoji dropdown already does if it fails to fetch the async chunk
• loading state could be communicated via the blue bar up top (if it's not already)
• I'm a bit worried about continuing our reliance on ReactRouter v2. React already complains quite a bit in the console about it, and apparently it won't even work with React 16 once that's released. Any chance we could at least upgrade to v3? Or should that be a separate concern?

[nolanlawson]

why 3?

[sorin-davidoi]

Because there are three columns. Refactored to be cleared and not rely on a magic constant.

[nolanlawson]

These webpackChunkName comments get repeated enough that I wonder if it wouldn't make sense to just create an async_routes.js or something that contains all the import()s in one place.

[nolanlawson]

Also thanks for the WebpackBundleAnalyzer view, didn't realize the onboarding modal especially was so big! Nice fix to not require it for every user every time.

[nolanlawson]

Hm, any idea why certain common modules aren't being put in the common chunk? Looking at the features chunks in the bundle analyzer, it seems like modules like status.js and status_action_bar.js are being repeated in each chunk?

[sorin-davidoi]

Could it be because of the dynamic imports (I'm thinking that Webpack puts stuff in the common chunk for static imports, with the assumption that if you use dynamic imports you don't want this magic by default)? Just a hunch, a quick search did not reveal much.

Edit: seams like that is the case.

[sorin-davidoi]

The emoji dropdown has its own loading state which gets set to false if the fetch fails. I introduced new actions for fetching bundles, which will show the loading bar and an error if the fetch fails.

The only thing is that it is very tedious to work with, since we cannot abstract import statements. I would like to write something like:

fetchBundle('features/compose').then(Component => COMPONENTS.Compose = Component.default);

with fetchBundle handing the actions and fail state.

But since import requires some static information (basically the path), we have to do something like:

store.dispatch(fetchBundleRequest());
import('features/compose')
  .then(Component => {
    COMPONENTS.Compose = Component.default;
    store.dispatch(fetchBundleSuccess());
  })
  .catch(err => store.dispatch(fetchBundleFail(err)));

I've added the loading bar / toasts to the emoji picker as well.

[sorin-davidoi]

I will also add "dummy" empty columns while the bundles are being fetched. Currently the right-most column is being push to the right as the bundles come in

[Gargron]

Is this an upgrade of react-router? It kinda looks similiar to what I've seen from latest version of react-router, so maybe it wouldn't be hard to upgrade?

[Gargron]

Also slightly concerned if this is going to make #3207 harder

[sorin-davidoi]

@Gargron What if we keep this PR for "some components" and tackle the routes in another PR (after #3207 is merged)? Might also be worth discussing, as @nolanlawson mentions, if we should try to upgrade react-router before lazy-loading the routes.

[nolanlawson]

"some components" sounds reasonable to me; e.g. the onboarding model seems especially good to extract.

As for react-router, yeah, I sorta feel like any routing changes should wait until that upgrade is complete. I'm not sure if we want to go with v3 or v4, though; I coulda sworn I read somewhere that v3 is still under active development.

Ah wait, looks like we are on react-router v3 now: #3067. Not sure if it's still so urgent to upgrade to v4.

[sorin-davidoi]

Alright, closing this and opening another PR for "some components".

[sorin-davidoi]

Taking another shot at this.