refactor: migrate from Express/Node.js to ElysiaJS/Bun#55
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates the project from Node.js/Express to Bun/ElysiaJS and converts the codebase to TypeScript, including updates to Docker configuration, dependencies, and the test suite. Feedback identifies critical issues such as an invalid @types/node version, dead code in configuration fallbacks, and a regression in security metadata for email notifications. Additionally, recommendations were provided to optimize the Dockerfile, remove unused dependencies, update outdated npm scripts, improve type safety in plugins, and adhere to RESTful status code conventions.
| "@commitlint/cli": "^17.4.4", | ||
| "@commitlint/config-conventional": "^17.4.4", | ||
| "@types/jsonwebtoken": "^9.0.10", | ||
| "@types/node": "^25.6.2", |
There was a problem hiding this comment.
| const port = parseInt(getMust("HTTP_PORT") || "8080"); | ||
| const hostname = getMust("HTTP_HOSTNAME") || "0.0.0.0"; |
There was a problem hiding this comment.
The || "8080" and || "0.0.0.0" parts are dead code because getMust() will throw an error if the environment variable is not set, so the fallback values will never be used. To provide a fallback, you should import and use getFallback() from ./config instead.
| const port = parseInt(getMust("HTTP_PORT") || "8080"); | |
| const hostname = getMust("HTTP_HOSTNAME") || "0.0.0.0"; | |
| const port = parseInt(getFallback("HTTP_PORT", "8080")); | |
| const hostname = getFallback("HTTP_HOSTNAME", "0.0.0.0"); |
| /** | ||
| * Verify user's identity and issue an access token by a code | ||
| */ | ||
| .patch("/", async ({ body, set, error }: any) => { |
There was a problem hiding this comment.
When sending a notification email after a successful login with an email code, the sessionIp and accessUa are hardcoded to "unknown". This is a regression from the previous implementation and removes valuable security information from the notification email. You should pass request and server to the handler's context to get the real IP address and User-Agent.
For example, change the handler signature to async ({ body, set, error, request, server }: any) and then use getIPAddress(request, server) and getUserAgent(request.headers, true).
| RUN addgroup \ | ||
| -g 3000 \ | ||
| recv | ||
| RUN adduser -HD \ | ||
| -u 3000 \ | ||
| -G recv \ | ||
| -h /app \ | ||
| recv |
| @@ -7,23 +7,27 @@ | |||
| "main": "app.js", | |||
There was a problem hiding this comment.
The main field still points to app.js, which has been deleted in this refactoring. While it might not be used since you're running the app with bun src/index.ts, it's good practice to keep this field accurate. You could either remove it or point it to the new entry point.
| "main": "app.js", | |
| "main": "src/index.ts", |
| "lint-staged": { | ||
| "*.js": "eslint" | ||
| }, |
| "express": "^4.18.2", | ||
| "express-validator": "^6.14.3", |
| }) | ||
| .macro(({ onBeforeHandle }) => ({ | ||
| access(requiredRole: string | null) { | ||
| onBeforeHandle(({ auth, error }: { auth: AuthContext | null, error: any }) => { |
There was a problem hiding this comment.
The context object for the onBeforeHandle hook is typed as any. Elysia provides types for its lifecycle hooks (e.g., BeforeHandleContext) that can be used for better type safety and autocompletion. Using explicit types would improve the maintainability of the code. This is a general suggestion that applies to many of the new route handlers and plugins in this PR. For example: onBeforeHandle((context: BeforeHandleContext) => { ... }).
| user.roles = [...user.roles, body.role_name]; | ||
| await user.save(); | ||
|
|
||
| return { message: "Role added" }; |
There was a problem hiding this comment.
| user.roles = user.roles.filter((name) => name !== params.role_name); | ||
| await user.save(); | ||
|
|
||
| return { message: "Role removed" }; |
There was a problem hiding this comment.
This DELETE route successfully removes a resource. It's conventional to return a 204 No Content status code with an empty body. The current implementation returns a 200 OK with a JSON body. You can achieve this by adding set to the handler's context, calling set.status = 204;, and removing the return statement.
…and clean up auth plugin syntax
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the application from Node.js and Express to Bun and ElysiaJS, involving a comprehensive rewrite from JavaScript to TypeScript. The changes encompass updated Dockerfiles, Mongoose models, and the implementation of Elysia-specific plugins for authentication and rate limiting. Feedback correctly identifies critical runtime issues where status is incorrectly invoked as a function instead of using set.status, which will lead to failed responses. Additionally, the review points out potential unique constraint violations during soft deletes, the use of t.Any() which compromises validation for sensitive credentials, and fragile status code parsing in the restrictor plugin.
| .get("/users/:user_id", async ({params, status}) => { | ||
| const user = await User.findById(params.user_id).exec(); | ||
| if (!user) return status(404); |
There was a problem hiding this comment.
In ElysiaJS, status is not a function in the context; it is typically set via set.status. Calling status(404) will likely result in a runtime error. Furthermore, returning a numeric value from a handler causes Elysia to use that number as the response body while keeping the HTTP status as 200 OK. You should set set.status explicitly and return the desired response body (or nothing). This pattern is repeated in several other route files.
.get("/users/:user_id", async ({params, set}) => {
const user = await User.findById(params.user_id).exec();
if (!user) {
set.status = 404;
return;
}| onBeforeHandle(({auth, status}: any) => { | ||
| if (!auth || !auth.id) { | ||
| return status(401); |
There was a problem hiding this comment.
Similar to the route handlers, status is not a function in the context. In onBeforeHandle, returning a value will stop the request and use that value as the response body. To return an empty 401 response, set set.status and return nothing.
| onBeforeHandle(({auth, status}: any) => { | |
| if (!auth || !auth.id) { | |
| return status(401); | |
| onBeforeHandle(({auth, set}: any) => { | |
| if (!auth || !auth.id) { | |
| set.status = 401; | |
| return; | |
| } |
| if (!user) return status(404); | ||
|
|
||
| user.nickname = APP_NAME; | ||
| user.email = new Date().toISOString(); |
There was a problem hiding this comment.
Using new Date().toISOString() as a replacement for the email during soft delete may lead to unique constraint violations if multiple users are deleted at the exact same millisecond. It is safer to include the user's ID to ensure uniqueness.
| user.email = new Date().toISOString(); | |
| user.email = `deleted-${user.id}-${Date.now()}`; |
| body: t.Object({ | ||
| session_id: t.String(), | ||
| credential: t.Any(), | ||
| }), |
| const currentStatus = typeof set.status === "string" ? | ||
| parseInt(set.status) : set.status; |
There was a problem hiding this comment.
No description provided.