-
Notifications
You must be signed in to change notification settings - Fork 148
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
ci: add bundle size report #1975
Conversation
|
✅ PR title follows Conventional Commits specification. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 84bac07:
|
4405a91
to
9c287f4
Compare
Bundle Size ReportUpdated Components
|
c74fb27
to
0635f48
Compare
'ActionListItemAsset', // This is just an <img> tag | ||
// These are same as Badge, Amount, Counter, Link, Text components | ||
'CardHeaderAmount', | ||
'CardHeaderBadge', | ||
'CardHeaderCounter', | ||
'CardHeaderLink', | ||
'CardHeaderText', |
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.
Any harm in keeping these? They are wrappers but their size might increase decrease depending on the code we add later no.
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.
It just increases the script run time, these components can add approx ~2mins. I believe these wrappers would hardly get updates. They have dev-only checks to verify amount is being used inside card only.
blade/packages/blade/src/components/Card/CardHeader.tsx
Lines 57 to 61 in 8d89eb5
const _CardHeaderAmount = (props: AmountProps): React.ReactElement => { | |
useVerifyInsideCard('CardHeaderAmount'); | |
return <Amount {...props} />; | |
}; |
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.
Add this in a comment for future reference. I was going to propose the same, to keep these components. But if it increases our CI time by 2 mins then we shouldn't
instead of ✅ and 🚫 can we use 🟢 and 🔴 to be more clear? |
The check seems to take 15 - 17minutes 😢 Is there any alternate way to implement or any way to optimize this? |
Yeah, this is the time taken by the size limit package. But this is expected because we have to initialize and bundle a webpack project for each component. Let me try to use m1 MacOS on CI. Otherwise, we can move to our custom implementation in the future but that would be a significant effort. |
2e5f5b2
to
9fd449b
Compare
packages/blade/package.json
Outdated
@@ -127,6 +127,7 @@ | |||
"watch:test:react-native": "yarn test:react-native --watch --onlyChanged", | |||
"chromatic": "npx chromatic", | |||
"publish-npm": "node ./scripts/publishToNpm.js", | |||
"bundle-size": "node ./scripts/generateBundleSizeInfo.js", |
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.
Call it generate-bundle-size-info
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.
Run the build command before generating a bundle size info. This way we can ensure that the generated info is always on the latest changes and not some old build on local
"bundle-size": "node ./scripts/generateBundleSizeInfo.js", | |
"bundle-size": "yarn build && node ./scripts/generateBundleSizeInfo.js", |
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.
Done. We don't need to run the entire build, just the react:prod
build. Added generate-bundle-size-info
& pregenerate-bundle-size-info
(this will automatically run before generate-bundle-size-info
)
'ActionListItemAsset', // This is just an <img> tag | ||
// These are same as Badge, Amount, Counter, Link, Text components | ||
'CardHeaderAmount', | ||
'CardHeaderBadge', | ||
'CardHeaderCounter', | ||
'CardHeaderLink', | ||
'CardHeaderText', |
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.
Add this in a comment for future reference. I was going to propose the same, to keep these components. But if it increases our CI time by 2 mins then we shouldn't
}); | ||
|
||
// Write the gathered size information to the specified file | ||
const filename = process.env.BUNDLE_SIZE_STATS_FILENAME || 'PRBundleSizeStats.json'; |
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.
Lets run prettier on this file once generated?
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 feel prettier is not required, we need to store json just for diff. Someone will hardly read 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.
It looks terribly ugly otherwise. Even if its just for diff, lets format it properly and store. It will hardly take any time
ci: fix size-limit check ci: add bundle size check
a20ce9a
9fd449b
to
a20ce9a
Compare
@@ -25,6 +25,6 @@ runs: | |||
if: steps.cache.outputs.cache-hit != 'true' | |||
|
|||
- name: Run postinstall script | |||
run: yarn postinstall && yarn workspace @razorpay/blade postinstall | |||
run: yarn postinstall |
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.
Why this change?
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.
postinstall script is no longer required, it was just running patch-package for bundlemon & we no longer depend on bundlemon.
Let's see how it goes 🙏🏻 |
Workflow
PR
PRBundleSizeStats.json
PRBundleSizeStats.json
withbaseBundleSizeStats.json
from the master and report the difference on PR via danger-js.master
baseBundleSizeStats.json
.baseBundleSizeStats.json
file.Addition info