-
Notifications
You must be signed in to change notification settings - Fork 49
Update the build and publish process #503
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
base: main
Are you sure you want to change the base?
Conversation
|
Cloud CI checks seem really flaky today: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Pull request overview
This PR modernizes the build and publish process by migrating to npm workspaces with per-package build directories and eliminating custom build scripts in favor of standard npm workspace commands.
Key Changes:
- Replace custom TypeScript build tooling (
.build/build_and_prepare.ts) with npm workspace-native commands - Implement per-package
tsconfig.jsonand build scripts that output to individualdistdirectories - Use
jqand npm's built-inversioncommand for dependency version management
Reviewed changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/client-web/tsconfig.json | Adds package-specific TypeScript configuration with dist output directory |
| packages/client-web/package.json | Updates version to 1.15.0, adds build/prepack scripts, pins client-common dependency |
| packages/client-node/tsconfig.json | Adds package-specific TypeScript configuration with dist output directory |
| packages/client-node/package.json | Updates version to 1.15.0, adds build/prepack scripts, pins client-common dependency |
| packages/client-common/tsconfig.json | Adds package-specific TypeScript configuration with dist output directory |
| packages/client-common/src/client.ts | Fixes self-reference imports to use relative paths instead of package name |
| packages/client-common/package.json | Updates version to 1.15.0, adds build/prepack scripts |
| package.json | Updates version to 1.15.0, removes old build script entries |
| CONTRIBUTING.md | Updates release process documentation for new workflow using npm workspaces |
| .scripts/update_version.sh | Adds new script for updating versions across packages using jq and npm |
| .scripts/build.sh | Removes old build script replaced by workspace commands |
| .github/workflows/build.yml | Simplifies CI build steps to use npm workspace commands |
| .build/build_and_prepare.ts | Removes custom build script replaced by standard npm tooling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [ -f "$package/package.json" ]; then | ||
| echo "Updating client-common version in $package/package.json" | ||
| json=$(cat "$package/package.json") | ||
| echo "$json" | jq --arg version "$version" '.dependencies["@clickhouse/client-common"] = $version' > "$package/package.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.
why not a js script to avoid jq requirement
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.
Fair point. I'm surely ok with switching back to a JS script if there is a preference on your side.
My reason is that in automation scripts the unix-way (i.e. using existing CLI tools) is the way to go. Also, jq is sort of a living standard these days (together with yq). This holds true, of course, until the complexity overgrows bash as a language in general.
Let me know if you'd like to switch and keep the pure JS approach here, I'll then make a follow-up PR.
| "description": "Official JS client for ClickHouse DB", | ||
| "homepage": "https://clickhouse.com", | ||
| "version": "0.0.0", | ||
| "version": "1.15.0", |
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.
does it need to be manually updated?
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.
Simply put, yes, which does not differ from the current approach.
The update is done by the script above (.scripts/update_version.sh). The current approach that is in main relies on changing 3 JS files and moving package.json around, while the new approach in this PR relies on updating two JSON files and leaving the rest to the npm CLI.
Potentially, with a more streamlined deploy pipeline we'd stick to only using npm without relying on jq and manually patching the dependency version. This IMO should come naturally when and if we're going with automating the deployment as @slvrtrn suggested as a north star here.
Why
To get ready to full migration to Vitest and also improve reliability by using a built version of the package in integration tests (as opposite to
tsx-ing it on the fly) we first need to switch to the npm standard workspace-aware build process where each package has its ownbuilddirectory and is built in order.This enables integration tests to use the as-close-as-it-gets package code during tests. Until we're running fully e2e tests with build-publish-install cycle (if at all needed) this PR is a step in the right direction.
What
npm --workspacesfeature to run workspace global commands likebuildandpackjqto bump theclient-commondependency in-placenpmbuilt-inversioncommand which keeps the JSON formatting the way npm likes it reducing diff-noise in case of other tools usedpathsoptionQA
I've built all three package using the old setup from main, packed the packages and in another repository checked in all three unpacked. To check if the new setup creates identical packages I then built all three using the new setup and extracted into the original folders in that other repository and checked the
git diffoutput. Except for the tiny code changes made for imports, the two newscriptsentries and the root path for source maps the were no other changes. See below.Going to test publishing (and fix issues if any) on the next release.