-
Notifications
You must be signed in to change notification settings - Fork 444
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
chore: Analyze NextJS bundle size action #2803
Conversation
Branch preview✅ Deploy successful! https://bundle_analyzer--walletweb.review-wallet-web.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Coverage report
Test suite run success1105 tests passing in 155 suites. Report generated by 🧪jest coverage report action from f913961 |
e832821
to
bcff8d0
Compare
push: | ||
branches: | ||
- dev |
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 we need 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.
According to the template this should be the default branch which is dev for us so it can compare bundle sizes with dev later on.
defaults: | ||
run: | ||
# change this if your nextjs app does not live at the root of the repo | ||
working-directory: ./ |
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.
This is the root, right?
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.
Yes, and I think its correct since the generated artifact includes all our routes and the global chunk size: https://github.com/safe-global/safe-wallet-web/actions/runs/6853583646
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 mean, you don't need to specify it as per the comment above.
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.
Ah ok, I will try to remove it completely and see if it works.
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.
Still works as expected 👍
- name: Restore next build | ||
uses: actions/cache@v3 | ||
id: restore-build-cache | ||
env: | ||
cache-name: cache-next-build | ||
with: | ||
# if you use a custom build directory, replace all instances of `.next` in this file with your build directory | ||
# ex: if your app builds to `dist`, replace `.next` with `dist` | ||
path: .next/cache | ||
# change this if you prefer a more strict cache | ||
key: ${{ runner.os }}-build-${{ env.cache-name }} |
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.
This should probably be in the build job in workflows/build?
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 think we can remove this part since our ./.github/workflows/yarn
already caches the result.
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.
Lovely jobly
What it solves
Resolves #2802
How this PR fixes it
Adds a new action to analyze bundle size changes of the nextjs app