-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Prune core-common js and lazy load modules #35432
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
Conversation
import Moment from './Moment' | ||
import CommentMixin from '../mixins/CommentMixin' | ||
const NcRichContenteditable = () => import('@nextcloud/vue/dist/Components/NcRichContenteditable') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we could fix in our libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Async components is a Vue feature. There's no bug, so there's nothing to "fix" as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i more meant if we can help all users of our libraries to do that out of the box, instead of having to bring it manually to all apps and points
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Whether a component can be lazy loaded depends on the assumptions the caller is making. For example, if some other function has some side effects that affects the component, lazy loading will break things. This is as out-of-the-box as it can get.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing worth figuring out would be the reason @nextcloud/vue
is so bloated (100KB per component at times). If that can be fixed then we don't need lazy loading at all. Another fix would be to stop bundling a lot of stuff in packages like @nextcloud/password-confirmation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplication that is in@nextcloud/vue
is getting addressed in #36383
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const NcRichContenteditable = () => import('@nextcloud/vue/dist/Components/NcRichContenteditable') | |
const NcRichContenteditable = () => import(/* webpackPreload: true */ '@nextcloud/vue/dist/Components/NcRichContenteditable') |
see https://webpack.js.org/guides/code-splitting/#prefetchingpreloading-modules
Signed-off-by: Varun Patil <varunpatil@ucla.edu>
Thanks for doing this and raising the topic @pulsejet So, there is a fine line between performance and lazy loading. Have you tried clicking the Loading an additional 1 or 2 MB on load, and considering it is cached, is actually not that bad. We have strong caching times. I haven't looked into details about your code yet, just putting this here for future discussions 😉 |
Please also calculate how long it takes to transfer all of those files. |
@skjnldsv this is less about performance and more about correctness. Loading everything for all pages in a core-common.js is wrong since most apps don't need most of the loaded components. A few examples:
Yes, I agree. Ideally, we should have a few timers loading these scripts asynchronously a few seconds after the page loads. This way the page loads fast enough but there's no slugishness. Still I'd maintain that loading everything at the start to combat this isn't the right solution. My server, for instance, has I'm willing to look into this later if this sounds good (but only if this gets merged first).
It's irrelevant. As I mentioned above, most of these files never get fetched at all in "normal" usage. For loading the Files app, nothing extra needs to be fetched with this PR. |
BTW another reason Nextcloud takes time to load in the browser is we load all apps scripts synchronously. For example, loading the viewer and sidebar scripts takes almost 200-300ms even on a fast machine. If we defer this to load a couple of seconds after the main view is loaded, the user is never going to know except for the better UX. |
AFAIK the problem is that many scripts are still not standalone. They depend on some global variables, like There was an attempt to make apps less dependent on these globals through the So I think the best we could do is add an option for apps to register their scripts as async-compatible and only load those asynchronously, at the end of the body. E.g. if the app Viewer works without OC/OCP/etc and no other script relies on the existence of global variables set by the Viewer app then the Viewer scripts could be loaded async. But making all scripts load async will likely cause problems, I'm afraid. |
Yeah, it isn't as easy as loading the scripts async. I doubt we can completely remove globals, but what I think would be ideal is load only the globals and critical parts synchronously and defer the rest to later (e.g. the Vue parts). |
Nextcloud is much more than the "files" app, my point still stand.
Fair points. But the issue still stands. Even as an admin.
|
Okay then point me to a place where 20 files need to be loaded and I can measure it. I have not found any such place. The one place where the most number of chunks need loading (that I found) is the user status modal (6 chunks). Loading all of them without cache takes ~150ms for me including network latency (~80ms) and JS parsing and execution. HTTP pipelining negates a lot of latency cost.
I have three points to make:
Do you mean it's fine to load admin-specific components for all users just so that the admin saves 200ms on some dialog (that too only on a cold cache)? I don't know what to say. Regardless,
Of course I have to mention this has risks of it's own. If the user is already interacting with the page (read scrolling) while we load the scripts, it has the potential to freeze up the UI creating even worse UX. When the user clicks on a button and there's a slight delay that's better because at least they might expect it. |
Obviously not 😁 |
Okay, so. I'm on your side, I'm the one that actually added the async loading in apps/users management and I'm also the one that added the UserStatusModal... I'm saying this because you can absolutely feel the sluggishness of the nextcloud load, even for tiny chunks. I don't have time to invest into this area for now. |
Understood. As I mentioned, it's a trade off to think about; I'll wait for reviews from others. Would be nice if more people can actually test this.
That's exactly my problem, just in a different place haha. The initial page load is too slow for me, and that happens every time I open nextcloud or switch to a different app. |
Nobody wants to review this? |
import { generateOcsUrl } from '@nextcloud/router' | ||
import { confirmPassword } from '@nextcloud/password-confirmation' | ||
import '@nextcloud/password-confirmation/dist/style.css' | ||
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() | |
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword() |
import axios from '@nextcloud/axios' | ||
import { confirmPassword } from '@nextcloud/password-confirmation' | ||
import '@nextcloud/password-confirmation/dist/style.css' | ||
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() | |
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword() |
import { generateOcsUrl } from '@nextcloud/router' | ||
import { confirmPassword } from '@nextcloud/password-confirmation' | ||
import '@nextcloud/password-confirmation/dist/style.css' | ||
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() | |
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword() |
import debounce from 'debounce' | ||
import NcColorPicker from '@nextcloud/vue/dist/Components/NcColorPicker' | ||
import Tooltip from '@nextcloud/vue/dist/Directives/Tooltip' | ||
const NcColorPicker = () => import('@nextcloud/vue/dist/Components/NcColorPicker') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const NcColorPicker = () => import('@nextcloud/vue/dist/Components/NcColorPicker') | |
const NcColorPicker = () => import(/* webpackPreload: true */ '@nextcloud/vue/dist/Components/NcColorPicker') |
import { confirmPassword } from '@nextcloud/password-confirmation' | ||
import '@nextcloud/password-confirmation/dist/style.css' | ||
import { print } from '../service/PrintService' | ||
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() | |
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword() |
<script> | ||
import NcButton from '@nextcloud/vue/dist/Components/NcButton.js' | ||
import NcEmojiPicker from '@nextcloud/vue/dist/Components/NcEmojiPicker.js' | ||
const NcEmojiPicker = () => import('@nextcloud/vue/dist/Components/NcEmojiPicker.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const NcEmojiPicker = () => import('@nextcloud/vue/dist/Components/NcEmojiPicker.js') | |
const NcEmojiPicker = () => import(/* webpackPreload: true */ '@nextcloud/vue/dist/Components/NcEmojiPicker.js') |
import NcEmptyContent from '@nextcloud/vue/dist/Components/NcEmptyContent' | ||
import moment from '@nextcloud/moment' | ||
const DashboardWidget = () => import('@nextcloud/vue-dashboard') | ||
const DashboardWidgetItem = () => import('@nextcloud/vue-dashboard') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems broken
|
||
<script> | ||
import MultiselectTags from '@nextcloud/vue/dist/Components/NcMultiselectTags.js' | ||
const MultiselectTags = () => import('@nextcloud/vue/dist/Components/NcMultiselectTags.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const MultiselectTags = () => import('@nextcloud/vue/dist/Components/NcMultiselectTags.js') | |
const MultiselectTags = () => import(/* webpackPreload: true */ '@nextcloud/vue/dist/Components/NcMultiselectTags.js') |
import RequestTime from './RequestTime' | ||
import RequestURL from './RequestURL' | ||
import RequestUserGroup from './RequestUserGroup' | ||
const RequestUserAgent = () => import('./RequestUserAgent') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RequestUserAgent = () => import('./RequestUserAgent') | |
const RequestUserAgent = () => import(/* webpackPreload: true */ './RequestUserAgent') |
import RequestURL from './RequestURL' | ||
import RequestUserGroup from './RequestUserGroup' | ||
const RequestUserAgent = () => import('./RequestUserAgent') | ||
const RequestTime = () => import('./RequestTime') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RequestTime = () => import('./RequestTime') | |
const RequestTime = () => import(/* webpackPreload: true */ './RequestTime') |
import RequestUserGroup from './RequestUserGroup' | ||
const RequestUserAgent = () => import('./RequestUserAgent') | ||
const RequestTime = () => import('./RequestTime') | ||
const RequestURL = () => import('./RequestURL') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RequestURL = () => import('./RequestURL') | |
const RequestURL = () => import(/* webpackPreload: true */ './RequestURL') |
const RequestUserAgent = () => import('./RequestUserAgent') | ||
const RequestTime = () => import('./RequestTime') | ||
const RequestURL = () => import('./RequestURL') | ||
const RequestUserGroup = () => import('./RequestUserGroup') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RequestUserGroup = () => import('./RequestUserGroup') | |
const RequestUserGroup = () => import(/* webpackPreload: true */ './RequestUserGroup') |
import { confirmPassword } from '@nextcloud/password-confirmation' | ||
import '@nextcloud/password-confirmation/dist/style.css' | ||
import { loadState } from '@nextcloud/initial-state' | ||
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword() | |
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword() |
Superceded by #38329 |
Enables lazy loading of libraries in the JS bundle. This improves perceivable UI loading time
by almost 2x(I realised my dev/test setup isn't ideal for testing this) in my test environment (to load the Files app) and reduces the transferred data from ~5MB to ~3MB. These results are shown beside the screenshots.The most painful things loaded in
core-common.js
(now separate):Caveat:
Production bundle before optimization:

After pruning:
