Migrate Spark APIi service to Fastify #Migrate our REST APIs to Fastify #549
Conversation
Goddhi
requested review from
bajtos,
juliangruber,
pyropy and
NikolasHaimerl
as code owners
@Goddhi CI is failing because package.json and package-lock.json aren't in sync. Could you please run |
| import { createHandler } from '../index.js' | ||
| import Fastify from 'fastify' | ||
| import { createFastifyApp } from '../fastify-app.js' | ||
| // import { createHandler } from '../index.js' |
There was a problem hiding this comment.
What is this code comment for?
| logger: { | ||
| level: 'info', | ||
| transport: { | ||
| target: 'pino-pretty' | ||
| } | ||
| } |
There was a problem hiding this comment.
| logger: { | |
| level: 'info', | |
| transport: { | |
| target: 'pino-pretty' | |
| } | |
| } | |
| logger |
To allow the caller of createFastifyApp to provide the logger
| client, | ||
| // Initialize Fastify app | ||
| const fastifyApp = await createFastifyApp({ | ||
| pgPool, |
There was a problem hiding this comment.
please update logger according to https://github.com/CheckerNetwork/spark-stats/blob/2558a45d7d17dd10a8671cfe8934ddfd55cf4363/stats/bin/spark-stats.js#L17-L19
| target: 'pino-pretty' | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
| }) | ||
|
|
||
| // Add shared context to each request | ||
| fastify.decorateRequest('pgPool', null) |
There was a problem hiding this comment.
Can we switch to @fastify/postgres instead of manual handling instead please?
There was a problem hiding this comment.
+1 on using @fastify/postgres
There was a problem hiding this comment.
@pyropy I have have made some commits after your review last week on this PR stats-api-pr, can you kindly help review once again, so I can replicate same @fastify/postgres implementation to this API as well, thanks.
| }) | ||
|
|
||
| // Error handler | ||
| fastify.setErrorHandler((error, request, reply) => { |
There was a problem hiding this comment.
Can we use fastify's default error handler instead? No need to keep the current implementation, which we only created, because we didn't have any default error handler
| }) | ||
|
|
||
| // Request logging | ||
| fastify.addHook('onRequest', (request, reply, done) => { |
There was a problem hiding this comment.
This can also be removed, as fastify is taking care of request logging now
| @@ -9,6 +9,7 @@ | |||
| "scripts": { | |||
| "start": "node bin/spark.js", | |||
| "lint": "standard", | |||
| "start:fastify": "node bin/spark-fastify.js", | |||
There was a problem hiding this comment.
| "start:fastify": "node bin/spark-fastify.js", | |
| "start:fastify": "node bin/spark-fastify.js", |
That file doesn't exist
There was a problem hiding this comment.
We also need to remove the old routes from https://github.com/CheckerNetwork/spark-api/blob/d3b87c2ae1e7bff084ab9cb36e4c5f3204f0369a/api/index.js
| measurement.providerId | ||
| ]) | ||
|
|
||
| return { id: rows[0].id } |
There was a problem hiding this comment.
| return { id: rows[0].id } | |
| reply.send({ id: rows[0].id }) |
There was a problem hiding this comment.
I suspect the other routes have similar issues. Once CI is fixed, that should show.
There was a problem hiding this comment.
If we are going to keep all routes in one file, then I prefer to call that file api/routes.js.
This PR migrates the Spark API server from the existing Node.js's built-in HTTP module to Fastify framework.
Changes Made
Added Fastify as a dependency
Created a parallel implementation that maintains compatibility with existing API contracts
Reorganized route handlers
Files worked on
api/bin/spark-fastify.js - modified this file for Fastify server entry point
api/fastify-app.js - Fastify application setup with middleware and hooks
api/routes/index.js - introduced this file for Route definitions migrated from api/index.js,
api/package.json - Modified this file by adding Fastify dependency and new start script