Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is type SerializerCompiler correct ? #47

Open
2 tasks done
remidewitte opened this issue Apr 2, 2024 · 5 comments · May be fixed by #48
Open
2 tasks done

Is type SerializerCompiler correct ? #47

remidewitte opened this issue Apr 2, 2024 · 5 comments · May be fixed by #48

Comments

@remidewitte
Copy link

remidewitte commented Apr 2, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

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 ?

@remidewitte remidewitte linked a pull request Apr 3, 2024 that will close this issue
4 tasks
@climba03003
Copy link
Member

@climba03003
Copy link
Member

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

cc @Eomm

@Eomm
Copy link
Member

Eomm commented Apr 3, 2024

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:
    return function buildSerializerFactory (externalSchemas, serializerOpts) {
  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

@Eomm Eomm linked a pull request Apr 3, 2024 that will close this issue
4 tasks
@remidewitte
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants