-
Notifications
You must be signed in to change notification settings - Fork 91
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 text-viewer and text-files #4698
Conversation
Thanks for looking into this. The change looks good to me, I just left one comment to ask @skjnldsv for 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.
So, the idea is nice, but we're reaching another issue. This is too much split and will cause another issue: too many requests.
The request load overhead does have a huge impact. It's good to be on-demand, but I think we should group the chunks together to load 2-3 big async files instead of the new 50+ files that are in this PR 🙈
875d1a2
to
07c2e0a
Compare
Indeed, most of it was due to mermaid. I added a rule to merge those together. For the rest, not sure, it is what the default webpack config does at the moment, if this is not what we want, then we should fix that upstream instead of here, no ? |
Some are really small, maybe |
We're getting close! 🚀 |
07c2e0a
to
bb7b5ea
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bb7b5ea
to
cdc219f
Compare
Might already work if we allow async components in https://github.com/nextcloud/viewer/blob/master/src/views/Viewer.vue#L814-L815 |
There is probably more that could be done with the file list plugin but this seems like a good first improvement, thanks for the help @pulsejet ❤️ |
Seems good from my perspective, the splitChunks.minSize does not seem to have an effect as there are still quite small chunks:
|
Cypress failures need some work as now it seems our default timeout for the editor is enough to lazy load all required js |
I've no idea how cypress works so can't help with that. But I've a request -- can we backport this to 28? This issue is absolutely killing the massive loading time improvements we got with 27 => 28 |
Sure, I'll have a look into the cypress stuff and would also backport to 28 still.
|
c0a6ee8
to
0a3da62
Compare
Actually timeouts are not the issue but outdated viewer js |
d530993
to
57f5e88
Compare
+ Remove vendor bundling + Remove chunkIds declaration as it is already the default + Group mermaid chunks Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
57f5e88
to
b9009dc
Compare
/backport to stable28 |
Fixes #2451
Fixes #5045
Currently, 5Mb are loaded on the main thread. The idea is to lazy load most of it, and let webpack do a proper chunking.
Also in this PR:
Current webpack config dates from: #1659