-
Notifications
You must be signed in to change notification settings - Fork 508
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
Add size-limit bundle size analysis tool #705
Conversation
Hi @agilgur5! please let me know if you have any thoughts about this! 🙏 |
Hey @andresz1 I definitely like the changes, thank you for the PR! I took a look when you first made the PR, but I just haven't had the time to dive deeper and review all the pieces of this. |
Hi @agilgur5! I'm glad you like the idea 🎉 and thank you for the reply. Let me know if you need help. Is there any |
Sorry for the delay on reviewing this, I've been incredibly stressed and busy, so larger PRs to add features got pretty backlogged as they take a good bit of time to really dive into.
Here's a few past references:
Summarizing,
Did a quick glance through on NPM trends and
That being said, it does have more dependencies like
Can probably stick to template initially as a testing ground and then move to a dependency later if there is enough usage. Though with #634 we're moving a bit away from dependencies.
I think this will require something like webpack to bundle pieces. Multi-entry it would need to show each entry and
In progress.
No, and I don't think that would be helpful either. Keeping discussions async and transparent on a PR is ideal. Watching those channels would take more time as well. |
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.
2 questions and a few changes needed to all CI workflows, but the rest looks good! Thank you for the PR and sorry for the wait.
env: | ||
CI_JOB_NUMBER: 1 |
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.
what's the purpose of this? I found https://github.com/ai/ci-job-number, but this doesn't run a matrix in any case; there is only one job here.
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.
I wasn't able to remember why this was needed 😱 I tried without it and the problem is that size-limit doesn't work properly in the action (it throws an error), so for now is needed. I'll try to investigate more about this
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.
Do you have a log of the error? Since this is the only job in this new workflow, making it go first shouldn't be necessary, but maybe the action relies on this variable being specified in order to work
Ah, also
|
Co-authored-by: Anton Gilgur <agilgur5@gmail.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Anton Gilgur <agilgur5@gmail.com>
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.
Thanks for iterating on this after the long delay @andresz1!
Changes look mostly good, a few modifications to the docs requested. There are also 3 unresolved comments above that didn't get fixed/responded to last time around. Once those are resolved this should be good to go!
@andresz1 I'd make the changes myself and merge but there were some unresolved questions remaining from previous review that I was hoping you could answer |
Co-authored-by: Anton Gilgur <agilgur5@gmail.com>
Co-authored-by: Anton Gilgur <agilgur5@gmail.com>
Co-authored-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 sorry I was on vacations and thank you so much for the great feedback! I just answered your questions and made so changes! |
@andresz1 no worries on the delay or if you're on vacation, everyone does that. Was just following up, especially since I couldn't find the answers to some of these myself. |
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.
Great work and iterations! thanks for investigating the issues -- we'll have to check back on the CI_JOB_NUMBER
comment but that can be in a separate PR or done in later comments, this is ready to merge 🙂
This unfortunately didn't make v0.13.x, but will be released in v0.14.0
@all-contributors please add @andresz1 for code, doc, example |
I've put up a pull request to add @andresz1! 🎉 |
…mer#705) - Add size-limit and size-limit-action to all templates - Use `@size-limit/preset-small-lib` by default - Add `yarn size` and `yarn visualize` scripts - Remove outdated references to rollup-plugin-size-snapshot Co-authored-by: Anton Gilgur <agilgur5@gmail.com>
Performance budget tool
This PR is an initial idea (something that I thought that maybe could be useful after @jaredpalmer tweet) of how a performance budget tool could be added automatically in every template. Having this kind of tool helps you to keep your library bundle size as small as possible. I like size-limit because besides the size it can calculate the time it would take a browser to download and execute your JS.
I added
@size-limit/preset-small-lib
by default but in case that library size increases, it could be easily replaced by@size-limit/preset-big-lib
(adding time).Bundle size check
Bundle analyze
GitHub action
The action added (size-limit-action) will execute
size-limit
a comment the PRs made with the comparison of it.Any feedback would be appreciated and I'd be more than happy to iterate over a solution if you like the idea. Thanks in advance and for the great tool!