feat: Migrating to bun #52
Conversation
package.json
Outdated
| @@ -8,11 +8,11 @@ | |||
| "test": "test" | |||
| }, | |||
| "scripts": { | |||
| "test": "npm run build:ts && tsc -p test/tsconfig.json && tap --ts \"test/**/*.test.ts\"", | |||
| "start": "npm run build:ts && fastify start -l info dist/app.js", | |||
| "test": "bun install && tsc -p test/tsconfig.json && tap --ts \"test/**/*.test.ts\"", | |||
There was a problem hiding this comment.
We don't need these redundant bun install directives with every command.
There was a problem hiding this comment.
By directives you mean the command itself, or the commands that are following the bun install command?
There was a problem hiding this comment.
The bun install commands preceding the actual command
prod.Dockerfile
Outdated
| COPY --from=builder /app/dist ./dist | ||
|
|
||
| CMD ["yarn", "fastify", "start", "-a", "::", "-l", "info", "dist/app.js"] | ||
|
|
||
| CMD ["bun", "fastify", "start", "-a", "::", "-l", "info", "dist/app.js"] |
There was a problem hiding this comment.
If you try running this command (without Docker), you're likely to find this won't work.
- We're no longer transpiling ts -> js
- Once you replace this app.js with app.ts, I expect you'll find that the
fastifyCLI does not support TypeScript files.
Possible solution: remove dependence on Fastify CLI by using fastify eject, then try and see if it works out of the box, if not we might need some more adjustments.
There was a problem hiding this comment.
Can we use bun build:ts first then let the command work normally, cause then it will be able to find dist/app.js?
Yes the fastify CLi does not support typescript files, by using fastify eject-ts there is a new server.ts file generated to let it function as a standalone app, Im figuring it out still.
Will update soon
|
Okay, will look into this and get back |
|
@ditsuke This was the way console looked with fastify CLI Now it looks like this: |
There was a problem hiding this comment.
Hi @AnshGupta01, thanks for your patience with the review. Overall the PR is really solid, only a few low hanging changes and it'll be good to merge :)
| @@ -8,12 +8,11 @@ | |||
| "test": "test" | |||
| }, | |||
| "scripts": { | |||
| "test": "npm run build:ts && tsc -p test/tsconfig.json && tap --ts \"test/**/*.test.ts\"", | |||
| "start": "npm run build:ts && fastify start -l info dist/app.js", | |||
| "test": "bun server.ts && tsc -p test/tsconfig.json && tap --ts \"test/**/*.test.ts\"", | |||
There was a problem hiding this comment.
| "test": "bun server.ts && tsc -p test/tsconfig.json && tap --ts \"test/**/*.test.ts\"", | |
| "test": "tsc -p test/tsconfig.json && tap --ts \"test/**/*.test.ts\"", |
| @@ -1,21 +1,19 @@ | |||
| FROM node:18-alpine as builder | |||
There was a problem hiding this comment.
Remove builder build stage, instead:
- Start from oven/bun:alpine as before
- Copy first the manifest and lockfile
- Bun install like below
- Copy all assets
- ✔️
| COPY --from=builder /app/package.json /app/yarn.lock ./ | ||
| RUN yarn install --frozen-lockfile --production | ||
| COPY --from=builder /app/package.json /app/bun.lockb ./ | ||
| RUN bun install --frozen-lockfile --production | ||
| COPY --from=builder /app/dist ./dist |
There was a problem hiding this comment.
Copy all files (COPY . .) since we're not building JS assets anymore
| // Register your application as a normal plugin. | ||
| app.register(import("./src/app")); | ||
|
|
||
| // delay is the number of milliseconds for the graceful close to finish |
|
Hi, I was on a break since November, I will get back to you soon with the requested changes! |
Hi there, there was some git issue with the previous PR, hence raising this new PR.
I've incorporated the requested changes, but im stuck with the dockerFile updates (Please help me out)