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

Merge headers #405

Open
lilouartz opened this issue Aug 17, 2024 · 4 comments · May be fixed by #406
Open

Merge headers #405

lilouartz opened this issue Aug 17, 2024 · 4 comments · May be fixed by #406

Comments

@lilouartz
Copy link

lilouartz commented Aug 17, 2024

These seems to be an issue, where if I send headers using Fastify, then anything that gets added by Remix is overriden, e.g.

Here I am adding link header:

await app.register(async (instance) => {
  instance.decorateRequest('cspNonce', null);

  instance.removeAllContentTypeParsers();

  instance.addContentTypeParser('*', (_request, payload, done) => {
    done(null, payload);
  });

  const handler = createRequestHandler({
    build: serverBuild,
    getLoadContext(request) {
      return {
        nonce: request.cspNonce,
        session: request.session as unknown as Session,
        visitor: request.visitor,
      };
    },
    mode: 'production',
  });

  instance.all('*', async (request, reply) => {    
    const links = getLinks(request.url);

    reply.header(
      'Link',
      [
        '</fonts/gabarito/Gabarito-VariableFont.woff2>; rel=preload; as=font; crossorigin',
        ...links,
      ].join(', '),
    );

    try {
      return handler(request, reply);
    } catch (error) {
      captureException({
        error,
        extra: {
          url: request.url,
        },
        message: 'could not render request',
      });

      return replyWithErrorPage(reply, error);
    }
  });
});

but the loaded route also send link header, which overrides my value from the above.

I would expect headers such as link to be merged.

@lilouartz
Copy link
Author

lilouartz commented Aug 17, 2024

Maybe something like this:

const sendRemixResponse = async <Server extends HttpServer>(
  reply: FastifyReply<Server>,
  nodeResponse: Response,
): Promise<void> => {
  reply.status(nodeResponse.status);

  for (const [key, values] of nodeResponse.headers.entries()) {
    // https://github.com/mcansh/remix-fastify/issues/405#issue-2471580896
    if (key.toLowerCase() === 'link') {
      const existingLinkValues = reply.getHeader('link');

      if (typeof existingLinkValues === 'string') {
        // We prepend Fastify headers rather than appending them because
        // Remix's `Link` header is more likely to contain route-specific
        // links, e.g. images. Meanwhile, Cloudflare appear to limit
        // the number of links that it extracts to Early Hints.
        reply.headers({ [key]: values + ', ' + existingLinkValues });
      } else {
        reply.headers({ [key]: values });
      }
    } else {
      reply.headers({ [key]: values });
    }
  }

  if (nodeResponse.body) {
    const stream = responseToReadable(nodeResponse.clone());

    return reply.send(stream);
  }

  return reply.send(await nodeResponse.text());
};

@mcansh
Copy link
Owner

mcansh commented Aug 17, 2024

hey @lilouartz thanks for info i'll poke around soon

@mcansh
Copy link
Owner

mcansh commented Aug 18, 2024

0.0.0-experimental-1d46df2 looks like there's some edge cases where headers are added multiple times as im not limiting it to just Link headers, but should be good for a test :)

@lilouartz
Copy link
Author

If I am reading this correctly, I am not sure if this desired behavior beyond Link example (and maybe select others), e.g. I wouldn't want x-robots-tag to be overriden.

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.

2 participants