Basic fastify package #24
Conversation
packages/emmett-fastify/src/index.ts
Outdated
| import closeWithGrace from 'close-with-grace'; | ||
| import Fastify, { type FastifyInstance, type FastifyRequest, type FastifyReply } from 'fastify'; | ||
|
|
||
| const defaultPlugins = [ |
There was a problem hiding this comment.
How these default plugins were chosen? I've seen etag in the express.js integration package as well as urlencoded, but there it was behind a flag, shouldn't we do something similar?
There was a problem hiding this comment.
I wouldn't go with flags. Imagine that you need 10 flags to sets. In my opinion will be better to choose some what you want. So rewrite existing setting using filter. But yeah worth to add something like name to filter by some array of names.
There was a problem hiding this comment.
I'm fine for now with weakly typed, but the feedback by @stepaniukm is valid. We should consider making it strongly typed. I'll also need to consider the general approach to plugins in Emmett.
For the context, I used flags intentionally as scarification. They're so ugly that if someone disables the default flags, then the bell should already ring, and they should ensure that's something they should do carefully. Still, of what I saw plugins should work for as it's aligned with Fastify conventions.
packages/emmett-fastify/src/index.ts
Outdated
| registerPreMiddlewares?: (app: FastifyInstance) => void; | ||
| registerPostMiddlewares?: (app: FastifyInstance) => void; | ||
| extendApp?: (app: FastifyInstance) => FastifyInstance; | ||
| activeDefaultPlugins?: Array<{ |
There was a problem hiding this comment.
That would be pretty nasty to use with TypeScript, notice how the default behavior of fastify.register(plugin, options) is, I mean you get autocomplete on the options, because of how it's typed, we should allow that here too with some generics magic (if possible) or maybe think of some better API for that purpose.
There was a problem hiding this comment.
Good call; still, I'm fine with handling that in the follow-up PR.
| }; | ||
|
|
||
| export interface ApplicationOptions { | ||
| serverOptions?: { logger: boolean }; |
There was a problem hiding this comment.
SUGGESTION: I think that we should allow people to provide their own server options and just set up the defaults to make configuration smoother. I wouldn't like to wrap the whole initialisation as then each change in fastify will trigger the need to adjust the ApplicationOptions.
packages/emmett-fastify/src/index.ts
Outdated
| export interface ApplicationOptions { | ||
| serverOptions?: { logger: boolean }; | ||
| registerRoutes?: (app: FastifyInstance) => void; | ||
| registerPreMiddlewares?: (app: FastifyInstance) => void; |
There was a problem hiding this comment.
SUGGESTION: Unless Fastify expects to have middleware registered in a certain order (e.g. before adding hooks, then I'd prefer not to have that middleware and extends here. The' getApplication' intends to provide the safe defaults and the most common configuration, but allow users to use native way of configuring tooling. So I'd prefer to just pass the settings that Emmett will be responsible for, so let user do something like:
const application = getApplication({apis: [shoppingCartApi(eventStore, getUnitPrice, () => now)]});
// do additional registration here
fastify.use(require('something')())Same comment apply to registerPostMiddlewares and extendApp.
packages/emmett-fastify/src/index.ts
Outdated
| ]; | ||
|
|
||
| const defaultPostMiddlewares = (app: FastifyInstance) => { | ||
| app.all('*', async (request: FastifyRequest, reply: FastifyReply) => { |
There was a problem hiding this comment.
CONCERN: Shouldn't we use setNotFoundHandler here? https://fastify.dev/docs/latest/Reference/Server/#setnotfoundhandler
Also shouldn't we have here also EmmetError mapping? I think that Fastify maps automatically statusCode field from error, and EmmettError has errorCode instead, so I think that it won't be automatically mapped.
There was a problem hiding this comment.
I will remove this to allow user use handling error how they want.
| await app.close(); | ||
| }); | ||
|
|
||
| app.addHook('onClose', (instance, done) => { |
There was a problem hiding this comment.
KUDOS: Nice, I need to add that also to Express 👍
There was a problem hiding this comment.
@mkubasz I added some comments/concerns/suggestions. Could you have a look at them? (and also do the rebase 😉 ).
Do you know if Fastify supports Problem Details out of the box?
Thanks for coming up with this PR; I think that once we agree on those comments and suggestions, we could pull it in.
feat: add some defaults plugins feat: start working on tests feat: working with types feat: finished test feat: finished test feat: update package
…o#29) * feat(docs,emmet-expressjs): add e2e api specification closes event-driven-io#26 * Configured E2E tests for using ESDB - Updated http responses to allow returning problem without options (e.g. NotFound) - Fixed path to ESDB package in tsconfig * refactor(docs,emmett-expressjs): apply review changes - Move ApiE2ESpecification to its own file - Move common test setup functions to utils file - Fixed typos "succeded" -> "succeeded" - Changed api to return not found when shopping cart does not exist * fix(emmett-expressjs): fix import statement --------- Co-authored-by: Oskar Dudycz <[email protected]>
mkubasz
force-pushed
the
feature/fastify-support
branch
from
3bcdd00 to
e84d4d7
Compare
There was a problem hiding this comment.
I'm fine with it as it is. We can tackle remaining items like problem details, integration and e2e test separately.
Btw. @mkubasz for the future, could you do rebase instead of merge? Then I won't need to do squash before merging :)
Fixed also linter errors
Goal of this PR is to prepare modular fastify package working with Emmett library.
Should contains default options, middleware and logger.
TBD...