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

Feat: Fastify adapter #87

Closed
SeanCassiere opened this issue Jul 24, 2022 · 27 comments
Closed

Feat: Fastify adapter #87

SeanCassiere opened this issue Jul 24, 2022 · 27 comments
Assignees
Labels
enhancement New feature or request

Comments

@SeanCassiere
Copy link
Collaborator

Not particularly the cleanest implementation, but I got an adapter working for Fastify.

https://github.com/SeanCassiere/fastify-trpc-openapi-adapter/blob/master/src/fastify.ts.

Let me know what you think. It's definitely a bit jank since the types for Fastify's request and reply objects don't exactly overlap all the types in NodeHTTPRequest & NodeHTTPResponse.

@jlalmes
Copy link
Owner

jlalmes commented Jul 26, 2022

Hi @SeanCassiere - thanks for this! We can add this fastify adapter to the trpc-openapi repo if you want to open an PR. Happy to work with you on this - we can use https://github.com/trpc/trpc/tree/main/packages/server/src/adapters/fastify as a battle tested pattern to follow.

@jlalmes jlalmes added the enhancement New feature or request label Jul 26, 2022
@jlalmes jlalmes mentioned this issue Jul 26, 2022
24 tasks
@jlalmes
Copy link
Owner

jlalmes commented Jul 26, 2022

I actually forgot to mention sachinraja/uttp when I wrote my earlier message. This could be a good solution - any thoughts?

@sachinraja
Copy link
Collaborator

@jlalmes I can try doing a PR with it if you want. I probably need to add some things before this package can use uttp anyway.

@jlalmes jlalmes changed the title Fastify adapter Feat: Fastify adapter Aug 16, 2022
@SeanCassiere
Copy link
Collaborator Author

SeanCassiere commented Aug 18, 2022

Sorry was out of sick for the last couple of days. Will get on this tomorrow.

As mentioned in your comment above, I'll match the pattern being used.

Any thoughts on the uttp thing? Or shall I do what I'm currently doing by remapping the request and reply the methods to match what is used by the Node HTTP service (https://github.com/SeanCassiere/fastify-trpc-openapi-adapter/blob/master/src/fastify.ts#L37-L42). @jlalmes

@jlalmes
Copy link
Owner

jlalmes commented Oct 14, 2022

Hi @SeanCassiere, sorry for the super late reply!

Or shall I do what I'm currently doing by remapping the request and reply the methods to match what is used by the Node HTTP service

If you're still keen to work on this then I think this is the route we should take :)

@SeanCassiere
Copy link
Collaborator Author

Hi @jlalmes, no sweat, I should have followed up on this as well.

I'll get on it 👍🏼.

@SeanCassiere
Copy link
Collaborator Author

@jlalmes just for confirmation, as this is for the 1.0.0 release - #91, I'll be pulling a branch off of next and NOT master.

@SeanCassiere
Copy link
Collaborator Author

SeanCassiere commented Oct 14, 2022

... the road so far

  • Added the adapter in this branch.
  • Added an example project, with tRPC, trpc-openapi, and swagger UI being set up in examples/with-fastify.
  • For the time being, I've copied over the same tests being used for the Express adapter, and altered the set-up process to run them on a fastify server with the new fastify adapter (which they are passing all of them 😊).
  • Added a super minimal set-up in the README.

@SeanCassiere
Copy link
Collaborator Author

@jlalmes in the testing of the fastify adapter, I've run into a blocker which requires more intimate knowledge on how the req and res objects are piped through the trpc-openapi plugin for the createContext function.

Scenarios

Please see the two scenarios below using Fastify:

1. Reading the headers in createContext

If I want to access the Authorization header using the @trpc/server/adapters/fastify plugin, I can access it using the following.

const createContext = async ({ req }: CreateFastifyContextOptions) => {
  console.log(req.headers.authorization);
  ...
}

However, in trpc-openapi, this is not possible since req.headers are undefined. As an alternative, the following has to be done to get to the header.

const createContext = async ({ req }: CreateFastifyContextOptions) => {
  console.log(req.raw.headers.authorization);
  ...
}

2. Settings headers in createContext

To set the x-request-id header with the @trpc/server/adapters/fastify plugin, it can be done using the following.

const createContext = async ({ res }: CreateFastifyContextOptions) => {
  const requestId = uuidv4();
  res.header('x-request-id', requestId);
  ...
}

However, in trpc-openapi, this is not possible since the res.header() function is not available, and instead the res.setHeader() has to be used.

const createContext = async ({ res }: CreateFastifyContextOptions) => {
  const requestId = uuidv4();
  res.setHeader('x-request-id', requestId);
  ...
}

Current fix/patch* in the createContext that works

Currently, in the createContext function, this can be worked around if you dropdown to using the raw objects stored in the req and res options.

const createContext = async ({ req, res }: CreateFastifyContextOptions) => {
  const requestId = uuidv4();
  res.raw.setHeader('x-request-id', requestId);
  if (req.raw.headers.authorization) {
    const user = jwt_decode(req.raw.headers.authorization);
  }
  ...
}

All-in-all, from what I can gather, the req and res objects being provided to the createContext function, are not the same as the ones being passed in for the primary tRPC fastify plugin.

@Asher-JH
Copy link

Hey @SeanCassiere, hows the blocker going? Would be awesome if we could use this fastify-adapter in an upcoming project soon. Anything I could help with maybe?

@SeanCassiere
Copy link
Collaborator Author

Hey @SeanCassiere, hows the blocker going? Would be awesome if we could use this fastify-adapter in an upcoming project soon. Anything I could help with maybe?

@Asher-JH, In it's current state the adapter is certainly usable, however, it wouldn't have the 'feel' of interacting with the first party Fastify Request and Response objects, rather you'd have to drop down to using the raw ones instead.

I've tried a few things to give it a shot, but I cannot understand why the Fastify methods are being stripped out when they arrive at the createContext function.

Need help from @jlalmes , as he's both the library author and quite a thorough understanding of how the official tRPC works.

@jlalmes
Copy link
Owner

jlalmes commented Oct 21, 2022

Looks great, thanks @SeanCassiere 🙌. Just opened this PR (#170) on your behalf - I will take a closer look over the weekend!

@keifufu
Copy link

keifufu commented Nov 2, 2022

I've tried a few things to give it a shot, but I cannot understand why the Fastify methods are being stripped out when they arrive at the createContext function.

Fastify's request and reply object are very funky when you try to clone them in any way.

Spreading the request and reply object removes all inherited/prototype stuff from it, so that didn't work.

Using Object.assign has some weird behavior if trustProxy is enabled, but it does work fine for the reply.

And for the request, we can pass it the original reference to openApiHttpHandler but set request.raw.url before (request.url is just a getter for request.raw.url)

Working snippet:

fastify.all(`${prefix}/*`, async (request, reply) => {
    const prefixRemovedFromUrl = request.url.replace(prefix, '')
    request.raw.url = prefixRemovedFromUrl
    return await openApiHttpHandler(
      request,
      Object.assign(reply, {
        setHeader: (key: string, value: string | number | readonly string[]) => {
          if (Array.isArray(value)) {
            value.forEach((v) => reply.header(key, v))
            return reply
          }

          return reply.header(key, value)
        },
        end: (body: any) => reply.send(body)
      }),
    )
  })

@SeanCassiere
Copy link
Collaborator Author

Thanks, @keifufu, I'll be pushing up your version of the call as it does look better without the spread syntax.


Overall, this Fastify Request object is proving to be quite fickle. I've also tried using structuredClone() for the Request as well, but it too leads to the same results.

It is truly quite frustrating, since accessing headers and cookies are something that could be done in the createContext as well as in the trpc-router procedures as well, and as such need to be consistent.

As of now, in its current state, the fastify-adapter for the plugin can be used in a project, HOWEVER, you'd have to ensure that all your queries, mutations, and middleware, access the request object items using req.raw.

@keifufu
Copy link

keifufu commented Nov 3, 2022

It is truly quite frustrating, since accessing headers and cookies are something that could be done in the createContext as well as in the trpc-router procedures as well, and as such need to be consistent.

Just making sure you understand that with the snippet I posted the objects don't get stripped anymore. Accessing headers, cookies and such works as expected. The changes I did were not done for looks :^)

@SeanCassiere
Copy link
Collaborator Author

@keifufu I appreciate the snippet you posted is not just for aesthetic purposes, but I think I may be missing something for the request object being held intact.

Could you confirm in the examples/with-fastify/src/router.ts file, that you are able to access the headers in the createContext function using req.headers, without dropping down to req.raw.headers?

This is the one I'm using to test it.

export const createContext = async ({
  req,
  res,
}: // eslint-disable-next-line @typescript-eslint/require-await
CreateFastifyContextOptions): Promise<Context> => {
  const requestId = uuid();
  res.raw.setHeader('x-request-id', requestId);

  let user: User | null = null;

  console.log('createContext req.headers', req.headers);
  console.log('createContext req.raw.headers', req.raw.headers);
  try {
    if (req.raw.headers.authorization) {
      const token = req.raw.headers.authorization.split(' ')[1];
      const userId = jwt.verify(token, jwtSecret) as string;
      if (userId) {
        user = database.users.find((_user) => _user.id === userId) ?? null;
      }
    }
  } catch (cause) {
    console.error(cause);
  }

  return { user, requestId };
};

You can test it using first the Swagger Docs http://localhost:3000/docs (this pipes the OpenAPI request into the trpc router), and then directly accessing the trpc query via the browser http://localhost:3000/trpc/posts.getPosts?batch=1&input=%7B%220%22%3A%7B%22json%22%3Anull%2C%22meta%22%3A%7B%22values%22%3A%5B%22undefined%22%5D%7D%7D%7D.

@keifufu
Copy link

keifufu commented Nov 3, 2022

@SeanCassiere The request doesn't get stripped anymore because we don't clone it (or spread, or anything). We just pass the reference from fastify.

I can't do any tests right now, or for the next few days, but I was able to access headers, cookies and any other variable from inside createContext just fine with the snippet I provided!

Edit: Yes, without using req.raw

@SeanCassiere
Copy link
Collaborator Author

@SeanCassiere The request doesn't get stripped anymore because we don't clone it (or spread, or anything). We just pass the reference from fastify.

I can't do any tests right now, or for the next few days, but I was able to access headers, cookies and any other variable from inside createContext just fine with the snippet I provided!

Edit: Yes, without using req.raw

Thanks!

I'll nuke my node_modules and maybe see if anything's going on with my install.

@LucasLundJensen
Copy link

LucasLundJensen commented Nov 25, 2022

Hello,
Is there any news in regards to this issue/PR? Seems like the issues with headers were solved by @keifufu. I'll happily test it out if required :)

@stale
Copy link

stale bot commented Jan 24, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 24, 2023
@NeilTheFisher
Copy link
Contributor

I'm curious as to why this hasn't been merged yet... I tested it by simply copying the file containing the plugin, registered it like in the example from the PR and it fine worked with a small modification to fix the url if it is nested in a fastify.register with a prefix:

fastify.all(`${prefix}/*`, async (request, reply) => {
-    const prefixRemovedFromUrl = request.url.replace(prefix, "");
+    const prefixRemovedFromUrl = request.url.replace(fastify.prefix, "").replace(prefix, "");
    request.raw.url = prefixRemovedFromUrl;
    return await openApiHttpHandler(

Here's how I'm registering it if anyone is curious:

fastify.register(
  async (fastify) => {
    // trpc main plugin
    fastify.register(fastifyTRPCPlugin, {
      prefix: trpcEndpoint, // "/api/v1/trpc"
      useWSS: true,
      trpcOptions: { router: appRouter, createContext },
    });

    // tRPC OpenAPI plugin to convert tRPC routes to normal ones
    fastify.register(fastifyTRPCOpenApiPlugin, {
      // basePath: "/",
      router: appRouter,
      createContext,
    });

    // Register Swagger UI
    fastify.register(import("@fastify/swagger"), {
      mode: "static",
      specification: { document: openApiDocument },
    });
    fastify.register(import("@fastify/swagger-ui"), {
      prefix: "/docs",
      uiConfig: {},
    });
  },
  { prefix: "/api/v1" }
);

@stale stale bot removed the stale label Jan 31, 2023
@SeanCassiere
Copy link
Collaborator Author

I'm sorry, I haven't been able to contribute and complete to this adapter because of current factors in my personal life.

If someone has the completed/working version of it, please push on the branch and let @jlalmes know.

@SeanCassiere
Copy link
Collaborator Author

Made the changes and tested. They have been pushed onto the branch.
#177

@NeilTheFisher
Copy link
Contributor

Nice! I was about to push to it with those changes and a unit test; it would have been my first open source contribution haha

Hopefully this gets merged soon

@SeanCassiere
Copy link
Collaborator Author

SeanCassiere commented Jan 31, 2023

Literally had an hour of off time today, and quickly pushed your code changes onto the branch.

There are some package-lock merge conflicts, but as soon as they are resolved, they can be merged onto main.

Edit: Have to head off now, @sylent3524 if you have time and can resolve the package-lock stuff, then it can probably be pushed today.

@stale
Copy link

stale bot commented Apr 1, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 1, 2023
@stale stale bot closed this as completed Apr 8, 2023
@jlalmes
Copy link
Owner

jlalmes commented May 24, 2023

Looking at the open PRs now 👀

Sorry for the delay - have disabled stale-bot!

@jlalmes jlalmes reopened this May 24, 2023
@jlalmes jlalmes removed the stale label May 24, 2023
@jlalmes jlalmes closed this as completed May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants