fix: Ability to modify request headers

[ARochniak]

fix #266

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
example-next-13-next-auth	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 11, 2023 6:54pm
next-intl-docs	✅ Ready (Inspect)	Visit Preview		May 11, 2023 6:54pm
next-intl-example-next-13	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 11, 2023 6:54pm
next-intl-examples-next-13	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 11, 2023 6:54pm

[vercel]

@ARochniak is attempting to deploy a commit to the Jan Team on Vercel.

A member of the Team first needs to authorize it.

[amannn]

Thank you very much for contributing this fix @ARochniak! 👏

[amannn]

This is not necessary for the current implementation in the main branch. The header is the workaround that is currently used in the RSC branch to "send" the matched locale to Server Components. It's also the reason why SSG doesn't work at this point in the RSC branch, I hope to get rid of this once ServerContext works well.

In the main branch we currently fully support SSG and don't need this workaround, therefore we shouldn't introduce it here.

Can you remove this?

[ARochniak]

@amannn Sorry, my bad! Sure I will remove it.

[amannn]

No worries! This is unfortunately a bit convoluted currently.

Btw. I'm currently in process of modernizing the code base a bit, the main branch now uses pnpm/turbo. Feel free to rebase to get a performance boost during development! The contributors guide is updated as well.

Next up is modernizing the test runner and build system …

[amannn]

See the comment above.

[amannn]

This is the fix that allows forwarding headers, right? Can you include a regression test in middleware.test.tsx that fails for the previous implementation and now passes?

[ARochniak]

Yes, sure. But need some time, if it's fine for you.

[amannn]

Sure, I'm really grateful for your contribution! Let me know if I can help!

[ARochniak]

Hi @amannn, while I was adding test case I realised that the previous solution was not good enough as it implicitly set request headers, even in cases when it's not necessary. But the current implementation also looks not clean enough for me. I would like to hear you opinion.
P.s. Also I realised how long ago I wrote tests 😁

[amannn]

Thanks for adding the test, hope you found your way around! 🙌

Good point that working with test runners might not be that common, I've just updated the contributors guide to contain some information in regard to running and writing tests. Hopefully this helps a bit!

[amannn]

Hmm, this doesn't look quite right. You're accepting a third parameter now for the middleware, but the goal is to keep existing incoming headers, right?

I think these tests could help to verify the implementation:

it('retains request headers for the default locale', () => {
  middleware(
    createMockRequest('/', 'en', 'http://localhost:3000', undefined, {
      'x-test': 'test'
    })
  );
  expect(
    MockedNextResponse.rewrite.mock.calls[0][1].request.headers.get(
      'x-test'
    )
  ).toBe('test');
});

it('retains request headers for secondary locales', () => {
  middleware(
    createMockRequest('/de', 'de', 'http://localhost:3000', undefined, {
      'x-test': 'test'
    })
  );
  expect(
    MockedNextResponse.next.mock.calls[0][0].request.headers.get('x-test')
  ).toBe('test');
});

These tests are set up to be placed at the spot in the test suite where you've added your test!

I've added another parameter for createMockRequest here to accept extraHeaders?: Record<string, string>.

If the goal of the consumer is to modify request headers, this can be done before the middleware is invoked.

Does this help? Let me know if I can help!

[ARochniak]

@amannn My initial intention was to provide ability to set custom request headers that can be consumed within server components. Previous implementation did it by implicitly forwarding headers from incoming request. But my concern is that it could go wrong at some point. As for me explicit setting of the responseInit looks more robust and extendable. That is how I see adjusted withExtraMiddleware example with explicit setting of the responseInit:

import createMiddleware, { NextMiddleware } from 'next-intl/middleware';
import { NextMiddlewareResult } from 'next/dist/server/web/types';
import {NextFetchEvent, NextRequest, NextResponse} from 'next/server';

export default withExtraMiddleware(
  createMiddleware({
    locales: ['en', 'de'],
    defaultLocale: 'en'
  })
);

type MiddlewareResponseInit = Parameters<typeof NextResponse.next>[0]

function withExtraMiddleware(next: NextMiddleware) {
  return async (request: NextRequest, event: NextFetchEvent) => {
    // Step 1: Potentially change the incoming request
    request.headers.set('x-test', 'test');

    // Step 2: Call the nested next-intl middleware
    const response = next(request, event, { request: { headers: request.headers } });

    // Step 3: Potentially change the response
    if (response) {
      response.headers.set('x-test', 'test');
    }

    return response;
  };
}

export const config = {
  // Skip all paths that should not be internationalized
  matcher: ['/((?!_next|.*\\..*).*)']
};

I hope I I was able to get my point across 😅 But if you don't see any issues with implicit forwarding headers from request I don't mind about it.

And thank you for the tests!

[amannn]

Thanks for explaining @ARochniak!

Previous implementation did it by implicitly forwarding headers from incoming request. But my concern is that it could go wrong at some point.

Do you have anything particular in mind that could go wrong? I think that by adding tests we can guarantee that this continues to work in the future.

Generally, if something can be solved with the existing APIs, I wouldn't want to add features. By doing this, we have to maintain less features within the library and also for the consumer there is a single way to achieve a particular goal (if we'd support a third argument, it's questionable what happens to existing request headers for example).

Therefore I'd really be in favor of not adding more API than necessary. Hope this makes sense!

[ARochniak]

@amannn Thanks for clarification. My concern is mostly based on some "God feeling" 😊. I haven't any issue about using previous approach, and I just wanted to discuss with you different option.

[amannn]

Looks great, thanks!

I've rebased your PR and fixed a lint issue. If tests pass, I'll merge this and I'll also publish a new RSC release rebased on top of this.

[ARochniak]

Looks great, thanks!

I've rebased your PR and fixed a lint issue. If tests pass, I'll merge this and I'll also publish a new RSC release rebased on top of this.

Thank you, it was a pleasure to contribute and communicate with you!

[amannn]

Thanks, the pleasure's all mine! 🙂

Here's the new RSC version (I think that's what you needed originally): 2.15.0-beta.3