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

Bundle optimization #5295

Merged
merged 47 commits into from
Apr 19, 2024
Merged

Bundle optimization #5295

merged 47 commits into from
Apr 19, 2024

Conversation

pnicolli
Copy link
Contributor

@pnicolli pnicolli commented Oct 7, 2023

Reduce bundle size by splitting code. Work started from editor/admin interfaces.

Already split as of this writing:

  • Folder Contents
  • Rules Controlpanel
  • Users Controlpanel
  • Relations Controlpanel
  • Form components
  • Diff

Reduced bundle size by ~7% (gzipped), from 582.92 to 542.6.
A lot more can be done.

@netlify
Copy link

netlify bot commented Oct 7, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 9218817
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/66216f456fe7b80009f69269

@pnicolli pnicolli marked this pull request as draft October 7, 2023 16:03
@pnicolli
Copy link
Contributor Author

pnicolli commented Nov 7, 2023

As of today, all controlpanels, form components and widgets are code split. We are down to 496kb gzipped (started from 582).

@pnicolli
Copy link
Contributor Author

pnicolli commented Nov 17, 2023

@tiberiuichim I added the babel plugin but I have a problem.

I added it to the razzle config and it works fine for building Volto with yarn, but it breaks jest (try to comment out the Header component in src/components/index.js to reproduce)

If I add the same configuration in babel.js then jest works fine but webpack breaks while building Volto.
Any ideas or pointers?

I pushed the current situation here in case you want to try it.

@pnicolli
Copy link
Contributor Author

After a discussion held during today's volto meeting, we decided to try to skip the babel transform imports plugin and moving all lazy loaded components back to the main components index and lazy load those there. I will try in the next days and let you know, so we can see how the result would be.

Copy link

netlify bot commented Dec 16, 2023

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 9218817
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66216f45a9855e00085570a1

@pnicolli
Copy link
Contributor Author

@sneridagh @tiberiuichim Removed babel-plugin-transform-imports and readded imports in components index. I realized I could import the lazyloaded components from their own index, and this looks a lot cleaner to me. This should also make this PR a non breaking change, actually. If it looks good for you, I will rebase onto the newest main branch for the final review.

@sneridagh
Copy link
Member

@pnicolli ok! let's talk about it tomorrow in the meeting. I have a couple of questions.

@stevepiercy stevepiercy self-requested a review April 17, 2024 17:25
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

FYI, in narrative text, we use one sentence per line. Also VSCode is overly aggressive with formatting .md files and should be turned off.

@stevepiercy
Copy link
Collaborator

This is really good work. After this is merged, then plone/documentation#1651 needs to be merged, so that an error about the missing term "lazy loading" will not throw an error. That PR also needs a quick review.

@stevepiercy
Copy link
Collaborator

I restarted the failed jobs, which might be due to flaky tests.

@stevepiercy
Copy link
Collaborator

It looks like a couple of Slate acceptance tests are broken.

@pnicolli
Copy link
Contributor Author

Yeah I am trying to fix these tests that didn't break a few days ago, and I can't replicate this locally even using Electron :( I tried restarting these multiple times but with no luck.
The test says cy.type('Colorless ...') and then checks that it is correctly there, but in the slate field you actually have the string Clorless... it happened locally once and never again, and even if it did I really think I would need help by slate-savvy people here. I keep getting errors with slate tests, but since they are really related to weird stuff I am wondering: am I really the only one getting these??

@ksuess
Copy link
Member

ksuess commented Apr 18, 2024

I can reproduce https://github.com/plone/volto/actions/runs/8726201362/job/23941446900?pr=5295 with Electron.
It types the wrong string 'Clorless'.
I cannot reproduce it with Chrome, which is used by the test workflow.
Nonetheless I suggest to try the fix that eliminates the failure locally.
I will do a PR with the fix of the test 'As editor I can add links' by using cy.getSlateEditorAndType. Also because the use of cy.get('#toolbar').click(); before typing does not make sense to me.

@ksuess
Copy link
Member

ksuess commented Apr 18, 2024

@pnicolli You may want to see that #5966 fixes the failing test https://github.com/plone/volto/actions/runs/8726201362/job/23941446900?pr=5295

@pnicolli
Copy link
Contributor Author

Green 💚
Thanks @ksuess for the final pointer on slate!
Thanks everyone. @sneridagh @tiberiuichim there a couple open conversations for you to check as a final approval.

Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

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

LGTM, I just tried with VLT and looks good too!

@tiberiuichim @ichim-david @davisagli @plone/volto-team please I'd like to merge this asap, it's that kind of PRs that enter in the merge hell due to the large file changeset, so I'd love if more people can take a look.

@sneridagh sneridagh requested a review from ichim-david April 18, 2024 18:48
@ichim-david
Copy link
Member

@pnicolli this is awesome work that should be merged as soon as possible and improved in future alpha releases in my opinion as @sneridagh says the more is left here the more work there is todo afterwards to merge the main branch here.

I do have one aspect that I would like to bring to your attention @plone/volto-team regarding the chunks names and I wonder if we shouldn't try to have a naming prefix, something like 'volto-core-' + 'chunk name'.
chunk-names
There is a big difference between the manual chunk name we give and what is assigned when there is chunk name comment as seen in the screenshot I took from the bundle analyzer.

I think the non named chunk naming is overkill and too verbose if I compare to the naming of other packages, yet I feel like having names such as Widgets, Contents is not very indicative that this is the volto core bundle and should have a prefix.

I leave this comment as a starting ramp for a potential brainstorming or opinion flood on the bundle naming as we all know that naming is one of the hardest problems in computer science :)

@pnicolli
Copy link
Contributor Author

@ichim-david fully agree that a more consistent naming should be adopted, my decision for this PR was to leave it for later since it would require more changes and I kind of wanted to draw the line at some point :D but I already planned more work on this topic

@sneridagh sneridagh merged commit 6f18663 into main Apr 19, 2024
74 checks passed
@sneridagh sneridagh deleted the bundle-optimization branch April 19, 2024 14:55
@sneridagh
Copy link
Member

Merged! @pnicolli @deodorhunter @mamico thanks a lot for this huge effort!! 💚

sneridagh added a commit that referenced this pull request Apr 29, 2024
* main: (49 commits)
  Update to Plone 6.0.11 (#5989)
  Defines the last 4 parameters of the `asyncConnect` function with optional (#5986)
  Flexibilize the pins for all ESlint deps, in Volto and generators (#5991)
  Release 18.0.0-alpha.29
  Release @plone/types 1.0.0-alpha.11
  fix: pass down locale to IntlProvider (#5976)
  Add Vite (client only, no SSR) build. Update Next.js 14.2.2 and Remix to 2.8.0 (#5970)
  Fix no router link in logo (#5981)
  Improve postinstall script, building all the deps (#5980)
  Better BlocksData types (#5979)
  Add missing types ts fields
  Release 18.0.0-alpha.28
  Release @plone/slate 18.0.0-alpha.12
  Release @plone/registry 1.5.6
  Improvements to the monorepo setup with utilities, especially ESLint.… (#5969)
  DEV: put nvm installation section into a separate include file (#5968)
  Bundle optimization (#5295)
  [client] Move provider to own package - Use parcel - `@plone/providers` (#5887)
  Fix flaky test 'As editor I can add links' by using getSlateEditorAndType (#5966)
  Fix rendering if ConditionalLink has no children (#5963)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants