Skip to content
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.

Sign up for GitHub

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

build:package-types: Run silently to reduce user confusion #61530

Merged
merged 2 commits into from
May 9, 2024
Merged

build:package-types: Run silently to reduce user confusion #61530

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8936853
build:package-types: Run silently to reduce user confusion
mcsf May 9, 2024
4917090
Keep just one script, delegate --silent flag to caller
mcsf May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it also suppress the error if it happens? Could we run something like --loglevel error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.js

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"build:plugin-zip": "bash ./bin/build-plugin-zip.sh",
"clean:package-types": "tsc --build --clean",
"clean:packages": "rimraf \"./packages/*/@(build|build-module|build-style)\"",
Expand Down
Loading