build:package-types: Run silently to reduce user confusion #61530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Follows up on #61501. The original PR, #61501, modified the `build:package-types` script to log a help message to developers if the `tsc --build` call failed. This message suggested a fix (to run `npm run clean:package-types`). This works, but due to the way that NPM logs the execution of successive NPM scripts, the code that conditionally outputs the help message is, itself, always output. This is not only noisy, but could mislead developers into following the help message's instruction unnecessarily. This commit fixes the problem by wrapping the original NPM script in a new script, which is then called with NPM's `--silent` flag. The console output now looks like this: > gutenberg@18.3.0 build:packages > npm run build:package-types && node ./bin/packages/build.js > gutenberg@18.3.0 build:package-types > npm run --silent build:package-types:verbose
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: 0 B Total Size: 1.74 MB ℹ️ View Unchanged
|
youknowriad
reviewed
May 9, 2024
package.json
Outdated
| @@ -268,7 +268,8 @@ | |||
| "scripts": { | |||
| "build": "npm run build:packages && wp-scripts build", | |||
| "build:analyze-bundles": "npm run build -- --webpack-bundle-analyzer", | |||
| "build:package-types": "node ./bin/packages/validate-typescript-version.js && ( tsc --build || ( echo 'tsc failed. Try cleaning up first: `npm run clean:package-types`'; exit 1 ) ) && node ./bin/packages/check-build-type-declaration-files.js", | |||
| "build:package-types": "npm run --silent build:package-types:verbose", | |||
| "build:package-types:verbose": "node ./bin/packages/validate-typescript-version.js && ( tsc --build || ( echo 'tsc failed. Try cleaning up first: `npm run clean:package-types`'; exit 1 ) ) && node ./bin/packages/check-build-type-declaration-files.js", | |||
There was a problem hiding this comment.
Do we really need two scripts, can we just keep one by default have an option to make it verbose or silent?
There was a problem hiding this comment.
Alternatively, printing the failure could go in another script:
tsc --build || npm run print-typescript-clean-suggestionThere was a problem hiding this comment.
Do we really need two scripts, can we just keep one by default have an option to make it verbose or silent?
If you don't have an issue with the delegation of responsibilities, I could keep just one script and modify the flags at the caller:
- "build:packages": "npm run build:package-types && node ./bin/packages/build.js",
+ "build:packages": "npm run --silent build:package-types && node ./bin/packages/build.js",
youknowriad
approved these changes
May 9, 2024
kevin940726
reviewed
May 10, 2024
| @@ -270,7 +270,7 @@ | |||
| "build:analyze-bundles": "npm run build -- --webpack-bundle-analyzer", | |||
| "build:package-types": "node ./bin/packages/validate-typescript-version.js && ( tsc --build || ( echo 'tsc failed. Try cleaning up first: `npm run clean:package-types`'; exit 1 ) ) && node ./bin/packages/check-build-type-declaration-files.js", | |||
| "prebuild:packages": "npm run clean:packages && lerna run build", | |||
| "build:packages": "npm run build:package-types && node ./bin/packages/build.js", | |||
| "build:packages": "npm run --silent build:package-types && node ./bin/packages/build.js", | |||
There was a problem hiding this comment.
Does it also suppress the error if it happens? Could we run something like --loglevel error?
There was a problem hiding this comment.
@kevin940726: As far as I can tell, it only suppresses NPM errors. For instance, npm run --silent foobar yields
npm error Missing script: "foobar"
npm error
npm error To see a list of scripts, run:
npm error npm run
npm error A complete log of this run can be found in: /Users/…/.npm/_logs/….log
It's not great but it's good enough for the problem. The thing is that I find it hard to justify spending any more time on this, especially since testing requires checking out a commit predating the type changes, clearing all type data, rebuilding everything, then jumping back to trunk and testing the script — and doing this for the different scenarios. The proper way to do it would be the way I initially drafted it: with a standalone script that lives in bin/ and orchestrates everything. I can write it by heart and I'm 95% this will work:
# bin/build-package-types
#########################
#!/bin/sh
# Exit if any command fails.
set -e
# Change to the expected directory.
cd "$(dirname "$0")"
cd ..
echo "Validating TypeScript version..."
node ./bin/packages/validate-typescript-version.js
echo "Building (`tsc --build`)..."
if ! tsc --build; then
echo 'tsc failed. Try cleaning up first: `npm run clean:package-types`'
exit 1
fi
echo "Checking build type declaration files..."
node ./bin/packages/check-build-type-declaration-files.jsThere was a problem hiding this comment.
with a standalone script that lives in
bin/and orchestrates everything.
Sorry I think I missed the convo. Why did we move away from this approach?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follows up on #61501.
The original PR, #61501, modified the
build:package-typesscript to log a help message to developers if thetsc --buildcall failed. This message suggested a fix (to runnpm run clean:package-types).This works, but due to the way that NPM logs the execution of successive NPM scripts, the code that conditionally outputs the help message is, itself, always output. This is not only noisy, but could mislead developers into following the help message's instruction unnecessarily.
This PR fixes the problem by requiring the caller script (
build:packages) to use NPM's--silentflag. The console output now looks like this: