fastify / fast-json-stringify-compiler

Build and manage the fast-json-stringify instances for the fastify framework
MIT License
8 stars 3 forks source link

Is type `SerializerCompiler` correct ? #47

Open remidewitte opened 7 months ago

remidewitte commented 7 months ago

Prerequisites

Issue

Hello,

From my research, I have the feeling that the declared SerializerCompiler might be wrong.

In fastify, compiler is eventually called this way :

serializerCompiler({
    schema,
    method,
    url,
    httpStatus,
    contentType
  })

In fast-json-stringify-compiler; with jfsOpts being already bounded,

function responseSchemaCompiler (fjsOpts, { schema /* method, url, httpStatus */ }) { ... }

My conclusion is that SerializerCompiler should be:

export type SerializerCompiler = (routeDef : RouteDefinition) => Serializer;

What do you think ?

climba03003 commented 7 months ago

Then the question is how https://github.com/fastify/fastify/blob/6564ba9b7d8ec7c236cc16f4bf3cdb3cabff69f0/lib/schema-controller.js#L159 be even possible?

remidewitte commented 7 months ago

Then the question is how https://github.com/fastify/fastify/blob/6564ba9b7d8ec7c236cc16f4bf3cdb3cabff69f0/lib/schema-controller.js#L159 be even possible?

I would say : https://github.com/fastify/fast-json-stringify-compiler/blob/main/standalone.js#L25

climba03003 commented 7 months ago

But it is not imported as standalone and the two version expect different arguments.

cc @Eomm

Eomm commented 7 months ago

Let's follow the happy path:

  1. fastify starts and the user has set 1 response schema at least
  2. fastify creates the default schema controller https://github.com/fastify/fastify/blob/c90ac14c7179606bb0833447dda1b83007156fa5/fastify.js#L214
  3. while creating the schema controller, the default serializer factory is instantiated https://github.com/fastify/fastify/blob/c90ac14c7179606bb0833447dda1b83007156fa5/lib/schema-controller.js#L27C40-L27C58
  4. Here is the factory function: https://github.com/fastify/fast-json-stringify-compiler/blob/6e8d53ac23618cd06e2c3caf806778098d810b8a/index.js#L6
  5. Now the server is starting and fastify processes the route with the json schema
  6. Since we don't have a FJS instance, the route asks to the schema controller to build a serializer https://github.com/fastify/fastify/blob/c90ac14c7179606bb0833447dda1b83007156fa5/lib/route.js#L420
  7. The serializer is built using the factory at step 3 (interface at step 4) https://github.com/fastify/fastify/blob/c90ac14c7179606bb0833447dda1b83007156fa5/lib/schema-controller.js#L159
  8. Finally we have the compile function generated by FJS and it is used to build the serializer-function https://github.com/fastify/fastify/blob/c90ac14c7179606bb0833447dda1b83007156fa5/lib/validation.js#L35

My conclusion is that SerializerCompiler should be:

It makes sense, but porting a real test case here will solve any doubt: https://github.com/fastify/fastify/blob/main/test/schema-serialization.test.js

remidewitte commented 7 months ago

Thanks for the fast feedback.

Indeed, in the happy path the compile function of type SerializerCompiler is called https://github.com/fastify/fastify/blob/main/lib/validation.js#L45 and https://github.com/fastify/fastify/blob/main/lib/validation.js#L45 In my opinion, it matches completely my initial suggestion.

About the real test case, as it is a js file and I am trying to solve a typing issue, I don't really understand what I could add to the PR, can you explain a bit more ?

Maybe less important httpStatus might be null : https://github.com/fastify/fastify/blob/c90ac14c7179606bb0833447dda1b83007156fa5/lib/reply.js#L382