-
Notifications
You must be signed in to change notification settings - Fork 0
Streamline dev/test with tsx and keep tsc build #59
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
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 refactors the project’s build and test workflow by switching production builds from tsc to an esbuild-driven script, and switching test execution to run TypeScript tests directly via tsx --test (no precompiled test output).
Changes:
- Add an esbuild-based build script (
scripts/build.js) to emit ESM JS intodist/src. - Update test helper to construct Fastify instances directly (avoiding
fastify-cli/helperundertsx). - Update
package.jsonscripts/deps and lockfile to includeesbuildandtsx.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
yarn.lock |
Adds lock entries for esbuild, tsx, and transitive deps. |
test/helper.ts |
Replaces fastify-cli test helper usage with direct Fastify instance setup. |
scripts/build.js |
New esbuild build script that compiles src/**/*.ts into dist/src. |
package.json |
Updates build/test scripts and adds esbuild + tsx dev dependencies. |
Comments suppressed due to low confidence (1)
package.json:18
yarn devstill runstsc -wwhilebuildis now esbuild-based. Sincedev:startserves fromdist/src/app.js, changes during development will be emitted bytsc(not esbuild), which can lead to runtime differences vs the production build pipeline this PR introduces. Consider updatingwatch/devto use an esbuild watch mode (or otherwise ensure dev and prod builds use the same compiler/output semantics).
"build": "node scripts/build.js",
"build:test": "yarn build && node scripts/copy-test-assets.js",
"watch": "tsc -w",
"start": "fastify start -l info dist/src/app.js --options",
"dev": "yarn run build && concurrently -k -p '[{name}]' -n TypeScript,App -c yellow.bold,cyan.bold yarn:watch yarn:dev:start",
"dev:start": "fastify start --ignore-watch='.ts$' -w -l debug -P dist/src/app.js --options",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread. In addition, remove all unused things (scripts, files, etc.) |
Applied the review feedback and removed the unused build:test script and copy-test-assets helper in 35e50c8. |
|
@polyipseity I'm thinking to run dev and tests directly (via I'm still not sure if we should touch build (i.e., change it to esbuild). It seems that there is no benefit of doing so. WDYT? |
Co-authored-by: FlandiaYingman <9929037+FlandiaYingman@users.noreply.github.com>
Co-authored-by: FlandiaYingman <9929037+FlandiaYingman@users.noreply.github.com>
Co-authored-by: FlandiaYingman <9929037+FlandiaYingman@users.noreply.github.com>
Co-authored-by: FlandiaYingman <9929037+FlandiaYingman@users.noreply.github.com>
943826a to
531f3bc
Compare
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.
@polyipseity I'm thinking to run dev and tests directly (via
tsxor something else); type checking them should be in a separate script. Benefits include quicker dev and tests, and we don't have to copy the assets everywhere.I'm still not sure if we should touch build (i.e., change it to esbuild). It seems that there is no benefit of doing so. WDYT?
Yeah, the tsx approach is good. (Also removes the need to copy assets). Use tsc --noEmit to type check without producing files to dist.
For esbuild, we don't benefit from it because bundling is incompatible with autoload. esbuild is probably faster than tsc but it skips type checking. Using tsc like the old approach keeps type checking, and is also simpler (no separate build system).
If we keep using tsc for build, one thing that could be improved is that in the script for build (and also dev), we should empty the dist directory first to ensure no stale files affecting autoload.
The remaining changes are good!
Yes that's always been an issue that annoys me when it's the case for |
It's still good to set it for consistency (and in case you run |
|
Based on the following discussion of this PR, design and implement the features and fixes. |
Co-authored-by: FlandiaYingman <9929037+FlandiaYingman@users.noreply.github.com>
Refactor the workflow to run dev and tests directly via tsx, while keeping production builds on tsc with a clean dist step.
tscfor production builds and cleandistbefore emitting to avoid stale autoloaded files.typecheckscript usingtsc --noEmitfor app and test type checking.tsx --testagainst.test.tsfiles without precompiling or copying assets.{ "scripts": { "build": "node scripts/clean-dist.js && tsc", "typecheck": "tsc --noEmit && tsc -p test/tsconfig.json --noEmit", "dev": "node scripts/clean-dist.js && FASTIFY_AUTOLOAD_TYPESCRIPT=1 tsx watch scripts/dev.ts", "test": "FASTIFY_AUTOLOAD_TYPESCRIPT=1 tsx --test --experimental-test-coverage test/**/*.test.ts" } }💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.