-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
69da60e
to
0a6bafa
Compare
bundle-analyzer.config.js
Outdated
}, | ||
{ | ||
test: '*.css', | ||
maxSize: '100 kB', |
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.
@koss-lebedev Nothing but just out of curiosity - How did we come up with these maxSize
values? Is it a standard? Thanks
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.
@HonzaBartos Well, the default setting for Bundle Analyzer is to cap all files at 250kb. I checked that the output from Gatsby for JS was under 200kb, so I thought it'd be nice to keep it that way 🙂The value for CSS size is random, just from experience, I think it should never grow over 100kb
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.
Afaik a standard for JavaScript is 170kB :)
I would leave there a note/todo, that this supposed to be ideally set in hand with performance budget. Even though this one kinda behaves as a perf budget, but it's not exactly it :)
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.
Looks solid to me 💪.
The only thing that I would add is .env.example
file to keep track of required env vars.
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.
Nice job! Looks straightforward. Kudos for readme! :)
I found a few nitpicks, otherwise LGTM.
bundle-analyzer.config.js
Outdated
}, | ||
{ | ||
test: '*.css', | ||
maxSize: '100 kB', |
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.
Afaik a standard for JavaScript is 170kB :)
I would leave there a note/todo, that this supposed to be ideally set in hand with performance budget. Even though this one kinda behaves as a perf budget, but it's not exactly it :)
@koss-lebedev could you also pretty please change the base of PR into v3? 🙏 |
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.
Ok, one more thing just popped in my mind. Could you please write down an ADR for that? I would put it into separate folder docs/adr
ADR helps to track other possible solutions. There is, for example, https://github.com/siddharthkp/bundlesize or https://github.com/webpack-contrib/webpack-bundle-analyzer
So it would be cool to know, why this bundle-analyzer was chosen :)
Thank you!
@dannytce Good question 🙂To be honest, I didn't consider other options because bundle-analyzer was referenced directly in the issue, but if you don't mind, I would like to take a look at other options |
@koss-lebedev yeah originally I wasn't planning to do that. But even with pretty straightforward Lighthouse #114 I realized there are numerous implementations of the same thing. So I thought it might be worth to try them and document them :) |
482f871
to
b61b092
Compare
b61b092
to
80426a8
Compare
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.
@koss-lebedev: Great job, love it! 💪
Two nitpicks
- 170kB was related to the JavaScript bundle, not CSS one 😅 Anyway don't worry about it, we can change whenever we want :)
- Could you also pretty please change the base of PR into v3? 🙏
This PR integrates with https://www.bundle-analyzer.com/ to check that file sizes are within the specified limit. I put 200kb as a max size for JS bundle, and 100kb for CSS. We can adjust them later, but for now they look like a good starting point.