-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Split main js files into modules and add them to the server bundle #13635
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
|
Loading the files app on master: 104 js files loaded And I'm not even done 🚀 |
9ffe3b2 to
887f2a0
Compare
|
I think I might stop at this point before this gets even less reviewable. It's possible to do this transition in another one or two PRs. |
|
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
1cb1674 to
6fa8844
Compare
|
@juliushaertl I've rebased onto master to resolve conflicts from #13407. Please have a look if I applied the changes correctly (should you find them in this huge diff). |
| import OCA from './oca'; | ||
| import escapeHTML from './util/escapeHTML' | ||
| import './jquery.avatar'; | ||
| import './jquery.contactsmenu'; |
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.
How about having a separate chunk for code that is only required when a user is logged in?
Besides the contactsmenu, when looking at core.json we probably also don't need to load systemtags/setupchecks on public pages.
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.
Sounds good! We can definitely try to load them only if required. We just have to check if the code still runs 😉
Looks good. |
|
Thanks for checking! I've now also added folders in |
|
Alright, this introduced a little 🐛 I'm unable to find 🔍. I'll do this in a bunch of smaller, more reviewable PRs :) |
No description provided.