Lazy loading components

[pnicolli]

First working draft. I left several comments that still need to be cleaned up, but I think they help to see what needs to be done to make a component load lazily.
At the moment, all of the main routes are split. In addition to those, also che Toolbar, ModalForm and Toast component are, as well as the File, Image and News Item views.

There are implementation issues that might lead to quite a lot of refactoring to add lazy loading to core volto components. See for example how many things I had to change in the src/components/index.js file. Every time a component has an import like import { Comp } from './components' webpack actually bundles all components in, so I had to refactor that to make it work. Many things in Volto core are grouped in an index file like that, therefore the refactoring issue I was talking about. Maybe we need to come up with a different approach for grouping components, for example for each split bundle we want.

For the same reason, I feel like current split bundles could be optimized by checking what actually is being split. Still, I believe we have a good starting point.

First things to do that come to mind:

• test in production mode
• maybe test more components

[pnicolli]

Also, there are now warnings during testing that say it would be better to upgrade React to 16.9^, is it something that's already planned?

[tisto]

@pnicolli thank you for your work on this! This is a major step for the bundle optimization and highly appreciated!

I guess upgrading to React 16.9 is on our todo list. Though, we should do so in a separate PR.

[tisto]

@pnicolli @sneridagh @robgietema this is technically a breaking change, correct? Do we have to assume ppl rely on those imports already? This has nothing to do with this PR really. We just have to somehow define our Volto API for semantic versioning/releases and upgrade guides.

[sneridagh]

@tisto It is, but for code splitting work as expected, we need to remove them which kind of sucks... Unless we find a better/elegant way.

[pnicolli]

Yeah, that's a breaking change :( Also, my current implementation is absolutely non-elegant, it would be awesome to find a better one. One components index per split bundle, for example, but that would mean having a better idea of what bundles we will split.

[sneridagh]

@pnicolli I'm a bit scared if the component overriding will continue working using loadable components, since we are removing the one single import point. We will have to experiment with it too.

I used the plane time home to play with it (I can't helped it!) and I've solved a problem in Html.jsx, can I push things there to improve it? Also, I couldn't get the routes working (it just do not create the chunk), did it for you? I will test your branch asap. I thought it's because how we use the routing (because of redux async in SSR, we should use react-router-config).

Thanks! Good work!!

[tisto]

@pnicolli I played around with this a bit (couldn't help even if I have some really urgent tasks on my todo list :)).

The lazy loading works! The bundle size is not significantly smaller yet (2.2 mb vs 2.3 mb non-gzipped). Though, that's fine for now.

I'd be also totally ok with just adding the lazy loading infrastructure first, merge it into master and then look into which parts we actually want to lazy load. I think the main target for this optimization is the Volto editor part (/edit /add routes). Maybe we should focus on this first and go step by step. What do you think?

[pnicolli]

@sneridagh Of course you can push things here. Also, from tomorrow to tuesday 5th I will be out of town and I'm not sure I will be able to contribute, so feel free to push stuff here even if I don't answer to comments during these days.
Routes work in this implementation, at first I noticed it wasn't splitting them, I had empty chunks, I thought I was doing configuration wrong but it was actually the fact that webpack was still bundling everything.

@tisto The bundle size is not dramatically reduced as you said. I like the idea of merging the configuration and basic implementation and iterate on that, though.

My gut feeling is that most of the things are reusing the same components, therefore webpack bundles most of the components in the main bundle anyways, so basically we only split the root components of routes, not everything under them. For example, I would love to try and lazy load every semantic-ui component inside the views and see what happens.

A good analysis tool would probably be a graph of the components and how they're tied to each other. The more it looks like a tree, the more it will be easy to reduce bundle size.

[sneridagh]

@pnicolli pushed my work in progress, now the tests blow up! I need to fix them. However, so far I get into this:

The vendor bundle is down to 350K :)
/cc @tisto

[tisto]

@sneridagh w00t! Seriously? That's so awesome!!!

[sneridagh]

@pnicolli now we have #1008 merged, we can continue with this :)

[sneridagh]

@pnicoli @tisto @cekk @giuliaghisini @nzambello

🎉 aaaaaaand it's green!! 🎉

@pnicolli I will amend your suggestion code might be a merge issue.

[pnicolli]

@sneridagh awesome! 🎉

[sneridagh]

@pnicolli BTW, I don't see how we can improve the test story :( I think that we have no other way to do it. The worse part is that we need to document it properly and that the overall DX complexity is raised one level.

[sneridagh]

I think it's ready.

[sneridagh]

@avoinea I would like to know your opinion on the current shape of this one.

[sneridagh]

@robgietema if you could take a look too before merging, it would be great!

[sneridagh]

@tiberiuichim raised some concerns about lazy load everything in Volto. I have to admit that I do have some concerns too about the final shape (raising complexity) of the current state and that it will lead to worsen the DX.

What about to have a meeting to discuss this before merging?

/cc @pnicolli @tiberiuichim @tisto @robgietema @nzambello @giuliaghisini @avoinea

[sneridagh]

Okay, I made some research, and I think I found something. Let me know what do you think:
https://codesandbox.io/s/loadable-async-tests-l2bx9

The gist is here:

import React from "react";
import { render } from "@testing-library/react";
import App from "./App";
import { Component1, Component2 } from "./components";

describe("CustomComponent", () => {
  it("rendered lazily", async () => {
    const { container, getByText } = render(<App />);

    await Component1;
    await Component2;

    expect(container.firstChild).toMatchSnapshot();
    expect(getByText("Component1"));
    expect(getByText("Component2"));
  });
});

The key is await for the loadable components in the tests. Yes, we will have to revisit all the tests, but might pay its price, since we will detect the ones that rely (unnecessarily) in child components. Then in the future we will be able to improve them bit by bit.

/cc @pnicolli @tiberiuichim @avoinea @giuliaghisini @nzambello @cekk @robgietema @tisto

[pnicolli]

Yes, that's exactly what I meant by "I feel like the only other solution would be to rewrite all the tests" 😁 I tried something very similar in the very first draft of the PR, that's why I added testing-library in the first place 😛

Regarding what to code-split, I actually kind of agree with @tiberiuichim. I said something similar on slack earlier, I thought I posted it here as well but actually I didn't 😞
Anyway, lazy loading external libraries and all views in some way, instead of every component, could save us from having the weird test-index file and having to rewrite every test.

I can see the benefit of having an entry point (the components/index) with all components already lazy loaded, so custom sites use them without caring about how lazy loading works, that's why I didn't push other ideas before. On the other hand, I feel weird about repeatedly downloading a whole lot of 10kb js files.

We could probably try to lazy load only the external libraries, the cms views (edit, controlpanels, etc), and probably the content-type views, even if it's probably not necessary since they reuse components already used in the other parts of the interface (mostly Semantic UI components and nothing more). I would be careful, though, to test a couple things properly:

• The developer experience: does something change in the way we create new views in custom sites? I didn't look into it, but I would like to see how the developer experience would change on that side.
• How is the testing experience this way around? How do custom site developers write tests? Is it the same as before?

Should we try to branch from here, change the lazy loading story like this (views and libraries) and manually try it?

[sneridagh]

Yes, that's exactly what I meant by "I feel like the only other solution would be to rewrite all the tests" 😁 I tried something very similar in the very first draft of the PR, that's why I added testing-library in the first place 😛

Yeah, kind of missed that :) I mixed things :( I was referring to rewrite to not rely solely on snapshots and alike.

Regarding what to code-split, I actually kind of agree with @tiberiuichim. I said something similar on slack earlier, I thought I posted it here as well but actually I didn't 😞
Anyway, lazy loading external libraries and all views in some way, instead of every component, could save us from having the weird test-index file and having to rewrite every test.

Yeah, the more I see it, the more I also think it. However, it can be hard to make
Webpack to really lazy load the things that we really want in a very specific, surgically way. Specially if we continue to use index.js.

I can see the benefit of having an entry point (the components/index) with all components already lazy loaded, so custom sites use them without caring about how lazy loading works, that's why I didn't push other ideas before. On the other hand, I feel weird about repeatedly downloading a whole lot of 10kb js files.

Maybe we should not allow core components to use it? Then Webpack will "see" only the specific imported components...

We could probably try to lazy load only the external libraries, the cms views (edit, controlpanels, etc), and probably the content-type views, even if it's probably not necessary since they reuse components already used in the other parts of the interface (mostly Semantic UI components and nothing more). I would be careful, though, to test a couple things properly:

+1

* The developer experience: does something change in the way we create new views in custom sites? I didn't look into it, but I would like to see how the developer experience would change on that side.

I tried it the lazy branch with a project and works well... In my projects I usually use the import {x} from '@plone/volto/components' way.

* How is the testing experience this way around? How do custom site developers write tests? Is it the same as before?

Outside core, I guess you can test your own components the same way as before, as long you do not require a Volto component that is lazy loaded...

Should we try to branch from here, change the lazy loading story like this (views and libraries) and manually try it?

I would do that, and play a bit more with it. At least to know what results we can get from it.

My proposal:

• PR adding support for lazy load for Volto, put together all that we need, the boilerplate, deps, etc make a PR with it.
• PR implementing a first use case using the dates widget alone lazy loaded (as we do now, in the index) - Amend tests to make it work, avoiding test-index.js

if it goes well...

• PR lazy loading all the routes, and libraries - Amend tests to make it work, avoiding test-index.js
• Document how to test async components properly, this is going to be mainstream when React Suspense is released, so better start now

then, from here, we can slowly lazy load components as we find it useful to do so, refrain if it does not make sense.

What do you think?

/cc @pnicolli @tisto @tiberiuichim @avoinea @robgietema

[sneridagh]

In fact, if from the user point of view, nothing changes... then we could live without marking it as breaking, right?

[rodfersou]

time to do a 😄 :

import this

+1 on the idea to use lazy loading just in the most important places if code gets more difficult do deal (debug and implementation)

[pnicolli]

100% love the plan :)
I agree completely that if nothing breaks how people develop their sites, there's no need to claim a breaking change.
The feeling I have right now is that actually the first time we change some tests to adapt to lazy loading, we are already breaking things, because if somebody has a snapshot test on a component that renders a lazy loaded component, their tests would break, I guess. Or not?

[sneridagh]

Closing this one, although we should remember it as reference for further improvements.