Lazy loading components

pnicolli commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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.

sneridagh commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

sneridagh commented 4 years ago

@pnicolli now we have https://github.com/plone/volto/pull/1008 merged, we can continue with this :)

pnicolli commented 4 years ago

@sneridagh Massive merge from master here. Locally I have all greens for jest tests and an error in cypress in the vimeo video block test which I have seen in other PRs, so I am tempted to think it's unrelated.

We should probably merge this before releasing Volto 4 final, if it looks good, I totally forgot this when we discussed possible other breaking changes.

pnicolli commented 4 years ago

On a side note, the path I have chosen to make tests work is weird, but solid, imho. I have duplicated the components index into a test-index.js which is used only by jest during tests, thanks to a name mapping. If we want to avoid this trick, we probably have to rewrite all 400+ tests in order to account for dynamic imports, which require some waiting on dynamic import promises to resolve.

I think we could go with the extra file I created, if we want to merge this before Volto 4 final. Removing it and modifying all the tests shouldn't break anything even if done after the release.

Thoughts?

sneridagh commented 4 years ago

@pnicoli I have my doubts to merge this for v4... Maybe sucha a refactor should be done in 5, although that means that 5 comes right after 4. I’m worried about how it works with a local project, and with local customizations. I’m also concerned about the dev experience and the lesrning story get too complex after the change...

I know that due to the nature of this change, the merges will be hellish.

How about this? Release 4, then a 5 alpha with the change, then start new projects with it (we have one on the move already).

As I’m writting, I’m a bit torn about this... Maybe we can meet next week and discuss this.

/cc @tisto

tisto commented 4 years ago

@pnicolli @sneridagh I would love to see this merged rather sooner than later. Though, it is indeed a possible breaking change. My proposal would be that we ship Volto 4 final in the next weeks and then do a 5 alpha release with the bundle optimizations. If that alpha works we can do a Volto 5 final soon as well. Does that sound ok to you?

pnicolli commented 4 years ago

@tisto @sneridagh I got back yesterday from a trip, I took some time to think about it while returning home.

If we keep this unmerged, the merges will not be easy, that's true, but this shouldn't be taken into account imho.

I'm fine with releasing Volto 4 without this and the start the 5 alpha with this change, possibly in a specific separate release branch, or something similar, since I expect that we will also support Volto 4, at least for important things.

sneridagh commented 4 years ago

@pnicolli I merged master with the branch, however, I couldn't make your way of doing tests work :( Any idea why?

sneridagh commented 4 years ago

@pnicolli ok, somehow, the merge with master failed in the most important line (in the jest modulemapper). I already fixed it.

sneridagh commented 4 years ago

@pnicolli well, there are in some points that I'm lost... I should have let you do the merge. Maybe we should talk about it.

What's the gist with the ModalForm? Why it was lazyloaded in the components it got called? In the merge I've put them all back to be get from components folder. Also, why the expected toBeEmpty ones?

When the async is returned, it's not supposed to be returning the (lazyloaded) component rendered? I can see it returns null, is that normal?

Maybe I'm too tired tonight! Will try in other moment!

Feel free to revert my merge!

sneridagh commented 4 years ago

@pnicoli @cekk @giuliaghisini It's green, but now we have some flaky tests again :(

tisto commented 4 years ago

@sneridagh flaky Cypress tests?

sneridagh commented 4 years ago

@tisto yes :(

sneridagh commented 4 years ago

I've been investigating a bit, and it seems that the problem is present only in the built environment. Locally I can reproduce the travis behavior (even in foreground mode). However, in dev environments, it works well :(

The problem seems that the setCookie doesn't work any more in a reliable way, don't have a clue why or what produces it. It happens in all (first time, that's the odd thing) calls to setCookie eg. cy.autologin() command.

Maybe trying to upgrade to 4.0.x cypress... I've checked the build, and it seems ok... :( also the built bundle seems fairly ok in my manual tests...

@tisto any idea?

sneridagh commented 4 years ago

@tisto @pnicolli the upgrade didn't work... :(

pnicolli commented 4 years ago

@pnicolli well, there are in some points that I'm lost... I should have let you do the merge. Maybe we should talk about it. Absolutely, we can talk about this.

What's the gist with the ModalForm? Why it was lazyloaded in the components it got called? In the merge I've put them all back to be get from components folder. Also, why the expected toBeEmpty ones? I think it was a leftover from my first tests, which involved Modals and a couple other components. I agree that if we choose to lazy-load everything from the components index, we don't need to do that in the ModalForm component.

When the async is returned, it's not supposed to be returning the (lazyloaded) component rendered? I can see it returns null, is that normal? IIRC it should, yes. Where d you see the null?

I'm still not 100% sold to using the test-index.js file, but I feel like the only other solution would be to rewrite all the tests.

Haven't had a chance to look at cypress, unfortunately.

sneridagh commented 4 years ago

@pnicoli @tisto @cekk @giuliaghisini @nzambello

:tada: aaaaaaand it's green!! :tada:

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

pnicolli commented 4 years ago

@sneridagh awesome! 🎉

sneridagh commented 4 years ago

@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 commented 4 years ago

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

sneridagh commented 4 years ago

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

sneridagh commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

[x] 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 commented 4 years ago

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

rodfersou commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

plone / volto

Lazy loading components #967