Remove bundle size check from pre-push hook #19993
Conversation
build-system/tasks/bundle-size.js
Outdated
| @@ -350,6 +351,14 @@ async function performBundleSizeCheck() { | |||
| if (argv.on_skipped_build) { | |||
| return await skipBundleSize(); | |||
| } else { | |||
| if (argv.on_pre_push_build) { | |||
| const files = gitDiffNameOnlyMaster(); | |||
| const runtimeFiles = files.filter(fileName => fileName.startsWith('src')); | |||
There was a problem hiding this comment.
This is a pretty crude check to see if a diff-ed file is in runtime. Let me know if there's a better one.
build-system/default-pre-push
Outdated
| @@ -25,7 +25,7 @@ | |||
| GREEN() { echo -e "\033[0;32m$1\033[0m"; } | |||
| CYAN() { echo -e "\033[0;36m$1\033[0m"; } | |||
|
|
|||
| GULP_BUNDLE_SIZE="gulp bundle-size" | |||
| GULP_BUNDLE_SIZE="gulp bundle-size --on_pre_push_build" | |||
There was a problem hiding this comment.
I initially added this to the pre-push hook because it was an instant local-only check that would take ~1 second to run. (Pre-push hooks should be fast.)
Now that we're doing more complicated stuff, should we consider removing this check from the pre-push hook? I'm not sure the check is designed to run from developer machines.
WDYT?
There was a problem hiding this comment.
I'd be totally cool with removing it altogether. =)
There was a problem hiding this comment.
Great. In that case, the only change needed is to delete this line :)
|
I think I'd rather remove this pre-push check entirely, since bundle-size checks are now actionable (i.e., someone can approve/reject a size increase on GitHub) |
cathyxz
changed the title
|
Thanks @rsimha and @danielrozenberg for the fast turnaround! Removed the |
* Skip check if no runtime files were touched * Apply this to pre-push builds only * Revert last 2 commits * Straight up delete bundle size check from pre-push hook
UPDATE: removing bundle-size check from pre-push hook altogether. Context / discussion below.
I'm getting this error message a lot in my pre-push hook on PRs that don't touch anything in runtime, causing my pushes to fail unnecessarily. Since we have a rate-limit and this check is pretty pointless for PRs that don't touch runtime, let's remove it from the default pre-push hook. =)