Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy load components #1675

Merged
merged 1 commit into from
May 24, 2023
Merged

Lazy load components #1675

merged 1 commit into from
May 24, 2023

Conversation

pulsejet
Copy link
Member

Filerobot bundles konva and react, and is absolutely massive. This is a very preliminary step, there's a lot more that could be cut down.

Related (but no dependency): nextcloud/server#38329

Before After
viewr-before viewr-after

@pulsejet pulsejet added 3. to review Waiting for reviews technical debt Technical issue performances Performances issues and optimisations labels May 21, 2023
@pulsejet pulsejet force-pushed the pulsejet/patch-webpack branch from 751f89f to d80ef81 Compare May 21, 2023 04:08
@pulsejet pulsejet force-pushed the pulsejet/patch-webpack branch 2 times, most recently from 911966e to a3850a7 Compare May 22, 2023 21:46
Comment on lines -171 to -173
import NcActionButton from '@nextcloud/vue/dist/Components/NcActionButton.js'
import NcActionLink from '@nextcloud/vue/dist/Components/NcActionLink.js'
import NcModal from '@nextcloud/vue/dist/Components/NcModal.js'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fond of lazy loading components that are always included. We don't gain anything but a bit more weight and network usage :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

127 0 0 1_8888_

Copy link
Member Author

@pulsejet pulsejet May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your call; I'd disagree though. The user preceivable performance is primarily due to JS (a) loading time and (b) parsing time. Splitting up the components bundle is faster on both counts. Since the bundle is prefetched and cached, the user can never know about the second request behind the scenes when the viewer is actually opened. In that sense, it's not really lazy loading, but just splitting up parsing and compilation of the critical and non-critical parts (since it all happens on the UI thread). Also the total size remains exactly the same as before, so no extra weight (note I'm looking at stat size, not parsed).

Copy link
Member

@skjnldsv skjnldsv May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the bundle is prefetched and cached, the user can never know about the second request behind the scenes when the viewer is actually opened.

That is not the case Webpack does not preload the js, it will load them on demand.
If you're on slow network, it can adds up quite a lot :)

Example given: 1.46s to wait for the viewer to load on 4G.
image

Copy link
Member Author

@pulsejet pulsejet May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you need to specify the webpackPrefetch and/or webpackPreload magic comment(s) to enable preloading. If that's not working then it's a bug maybe or prefetching is broken somehow.

https://webpack.js.org/guides/code-splitting/#prefetchingpreloading-modules

Copy link
Member Author

@pulsejet pulsejet May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Seems to work for me. If you turn on Disable Cache in the JS console then it disables prefetching; could be related to that. The above is with a cold cache in incognito.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work for me. If you turn on Disable Cache in the JS console then it disables prefetching; could be related to that. The above is with a cold cache in incognito.

Nope even with cache disabled 🤔
Component, plyr or editor are loaded after the viewer is opened.

I'm also ok merging this, and we can always see later how it goes 🤷
But I kind of would like a more wider discussion on async components, so we can all decide to go the same direction?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be up to create a thread on https://github.com/nextcloud/standards/issues with your already impressive graphs and data to show the benefits/disadvantages and how-to of this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope even with cache disabled 🤔

I meant it wont work with cache disabled 😅. But if it wasn't disabled earlier then definitely something is off 🤔

I'm also ok merging this, and we can always see later how it goes 🤷
But I kind of would like a more wider discussion on async components, so we can all decide to go the same direction?

Sounds good 👍🏻

Would you be up to create a thread on https://github.com/nextcloud/standards/issues with your already impressive graphs and data to show the benefits/disadvantages and how-to of this?

Will do. Never knew about this repo. I think most of the overhead is coming from nextcloud-vue at this point; desperately need tree shaking in there. If that can be fixed then lazy loading components would be pointless to begin with (happy dance).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. Never knew about this repo. I think most of the overhead is coming from nextcloud-vue at this point; desperately need tree shaking in there. If that can be fixed then lazy loading components would be pointless to begin with (happy dance).

This is on purpose, a bit more discrete to not become the place to discuss any thoughts. I think important topics like those should indeed be brought up there 👍

@skjnldsv skjnldsv removed the request for review from CarlSchwan May 23, 2023 12:09
@skjnldsv skjnldsv force-pushed the pulsejet/patch-webpack branch 3 times, most recently from f229755 to 8ad8710 Compare May 23, 2023 13:43
Signed-off-by: Varun Patil <[email protected]>
@skjnldsv skjnldsv force-pushed the pulsejet/patch-webpack branch from 8ad8710 to f9e9688 Compare May 24, 2023 09:01
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 24, 2023
@skjnldsv skjnldsv merged commit 51927fd into master May 24, 2023
@skjnldsv skjnldsv deleted the pulsejet/patch-webpack branch May 24, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish performances Performances issues and optimisations technical debt Technical issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants