Bundle optimization

[pnicolli]

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[bot]]

✅ Deploy Preview for volto canceled.

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

[pnicolli]

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

[pnicolli]

@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]

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.

[netlify[bot]]

✅ Deploy Preview for plone-components canceled.

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

[pnicolli]

@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]

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

[pnicolli]

@sneridagh @tiberiuichim @stevepiercy Marking this as ready for review. I added the docs we discussed a few months ago for core development, if you feel other docs are needed let me know. Should we mention this in the upgrade docs? It is transparent to the integrators imho, or at least it is aimed to be transparent and non-breaking.

I see a couple cypress tests are still not green, I'll keep looking into it but help is appreciated since I can't reproduce these locally :(

[ichim-david]

@pnicolli try to wrap the test in _.times, sometimes the tests are flaky and you might get the error only after x try. also try with electron and not a browser as I usually run cypress with Firefox and it's better than electron but since the tests run in electron you also need todo so in order to try to get the failures.

// run only this test 20 times Cypress._.times(20, (k) => { it.only('As editor, I can unlock a locked page', function () { console.log('test'); }); });

[pnicolli]

@ichim-david @tiberiuichim @sneridagh I managed to replicate the issue by using Electron (thanks for the pointer!) and I found that it is because of this line here: https://github.com/plone/volto/blob/main/packages/volto/cypress/support/commands.js#L707

I am not sure why typing a blank space then deleting it is needed, any ideas? Because only in electron and of course only in this branch the blank space is not deleted so the test breaks because of the extra space. I have no clue why changing the imports to lazy-loaded ones in slate (or in volto) would cause this behavior. On the other hand, I tried running all the tests that use the pasteClipboard command in this branch and they are all green by removing this line... but I'm not comfortable with just removing it without any confirmation, I don't know why it's there :)

[ichim-david]

@ichim-david @tiberiuichim @sneridagh I managed to replicate the issue by using Electron (thanks for the pointer!) and I found that it is because of this line here: https://github.com/plone/volto/blob/main/packages/volto/cypress/support/commands.js#L707

I am not sure why typing a blank space then deleting it is needed, any ideas? Because only in electron and of course only in this branch the blank space is not deleted so the test breaks because of the extra space. I have no clue why changing the imports to lazy-loaded ones in slate (or in volto) would cause this behavior. On the other hand, I tried running all the tests that use the pasteClipboard command in this branch and they are all green by removing this line... but I'm not comfortable with just removing it without any confirmation, I don't know why it's there :)

@pnicolli yup @tiberiuichim should probably think of why the empty space is still needed. I am looking at the changeset that introduced this command https://github.com/plone/volto/commit/0c40ed58cd7e9b78998ca8ccbcea53e417bdafd5#diff-1b1a73ba561eab6738c8b62510feed5b10edcd25c56626ac79017752b806439dR616 Usually when backspace it typed there is no need to have an empty space first so some clarification would be nice to have :)

[stevepiercy]

This is really good work. After this is merged, then https://github.com/plone/documentation/pull/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]

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

[stevepiercy]

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

[pnicolli]

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]

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]

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

[pnicolli]

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.

[ichim-david]

@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'.

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]

@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]

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