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

Restore original Vite chunk size limit #713

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dcsaszar
Copy link
Contributor

See https://v3.vitejs.dev/guide/build.html#chunking-strategy

I picked a chunking strategy where we chunk two large groups of vendor dependencies: React based and Scrivito based packages.

See https://v3.vitejs.dev/guide/build.html#chunking-strategy

I picked a chunking strategy where we chunk two large groups of vendor dependencies: React based and Scrivito based packages.
@dcsaszar dcsaszar requested a review from apepper December 17, 2024 08:44
@apepper apepper self-assigned this Dec 17, 2024
Copy link
Contributor

@apepper apepper left a comment

Choose a reason for hiding this comment

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

FEEDBACK (8531bb6)

Nice 🚀. I added some questions for discussion.

output: {
manualChunks: {
react: ['react-bootstrap', 'react-helmet-async', 'react-toastify'],
scrivito: ['scrivito-neoletter-form-widgets'],
Copy link
Contributor

Choose a reason for hiding this comment

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

FMI: Please help my understand, what this scrivito chunk means? My (maybe naive) understanding would be, that their is a chunk named scrivito, but it only contains scrivito-neoletter-form-widgets. But if I look at the resulting size it shows this:

dist/assets/scrivito-CFqY22Np.js                                     465.18 kB │ gzip: 144.80 kB

But scrivito-neoletter-form-widgets is not that big (should be ~13 kB gzipped). I assume, that it also compiles the npm package scrivito in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it includes dependencies. Same goes for the react-* packages, since they depend on react and react-dom. We could make it explicit, but since subdependencies are also included, it doesn't really make things clearer.

vite.config.ts Outdated Show resolved Hide resolved
outDir,
rollupOptions: {
input: {
main: resolve(__dirname, 'index.html'),
_scrivito_extensions: resolve(__dirname, '_scrivito_extensions.html'),
},
output: {
manualChunks: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Besides the newly created react and scrivito chunk there are still two js files in the end: main-<hex>.js and index-<hex>.js - since main-<hex>.js is quite small, can they be combined into one file?

outDir,
rollupOptions: {
input: {
main: resolve(__dirname, 'index.html'),
_scrivito_extensions: resolve(__dirname, '_scrivito_extensions.html'),
},
output: {
manualChunks: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any metrics that point to having the same (or even better) performance with manual chunks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to manual Lighthouse runs it's the same. Differences are all in the < 10 ms range.

@dcsaszar dcsaszar requested a review from apepper December 17, 2024 11:22
Copy link
Contributor

@apepper apepper left a comment

Choose a reason for hiding this comment

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

FEEDBACK (aa92913)

Almost there! Please see comments.

outDir,
rollupOptions: {
input: {
main: resolve(__dirname, 'index.html'),
index: resolve(__dirname, 'index.html'),
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, this only changes the name, but not merge the two files:

dist/assets/index-BZn5sF-c.js                                          6.56 kB │ gzip:   2.74 kB
dist/assets/editingConfigs-WubQ1JNa.js                                26.47 kB │ gzip:   8.25 kB
dist/assets/ScrivitoBootstrapIconEditor-DTXMz-zT.js                   47.51 kB │ gzip:  15.12 kB
dist/assets/index-DOwQ58pJ.js                                        107.26 kB │ gzip:  30.25 kB

Compare to one previous commit:

dist/assets/main-Bfrvei51.js                                           6.56 kB │ gzip:   2.74 kB
dist/assets/editingConfigs-WubQ1JNa.js                                26.47 kB │ gzip:   8.25 kB
dist/assets/ScrivitoBootstrapIconEditor-DTXMz-zT.js                   47.51 kB │ gzip:  15.12 kB
dist/assets/index-DOwQ58pJ.js                                        107.26 kB │ gzip:  30.25 kB

We can't use the simpler `index: ['index.html']` as this would also merge the CSS index assets which would load potentially unwanted app CSS from `_scrivito_extensions.html`.

An interesting side-effect of not using `index: ['index.html']` is that the `index.html` scripts are now all loaded by `<script>` tags instead of `<script>` plus `<link rel="modulepreload">`.

Next we could think about dropping the polyfill (https://vite.dev/config/build-options#build-modulepreload). The browser baseline is 2023 anyway (https://caniuse.com/link-rel-modulepreload).
@dcsaszar dcsaszar requested a review from apepper December 23, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants