QuiiBz / next-international

Type-safe internationalization (i18n) for Next.js
https://next-international.vercel.app
MIT License
1.26k stars 59 forks source link

Chain`next-international` middleware with others middlewares #187

Closed sshmaxime closed 12 months ago

sshmaxime commented 12 months ago

Is your feature request related to a problem? Please describe.

I'd like to chain middlewares in my app but I don't know how to do that with yours.

Describe the solution you'd like

Chainnext-international middleware with others.

Describe alternatives you've considered

This is nice tutorial about chaining middlewares but I cannot make it work with next-international middleware. https://reacthustle.com/blog/how-to-chain-multiple-middleware-functions-in-nextjs

QuiiBz commented 12 months ago

I cannot make it work with next-international middleware

Could you describe a bit more the problem here? Similar to the linked article, next-international's middleware accepts a NextRequest and returns a NextResponse, which follows the signature of NextMiddleware.

sshmaxime commented 12 months ago

Alright, I figured it out. I don't think it was actually related to your package, my bad.

For anyone wondering this is how I did it (following the tutorial above):

export const withI18n: MiddlewareFactory = (next: NextMiddleware) => {
  return async (request: NextRequest, _next: NextFetchEvent) => {
    await next(request, _next);
    return I18nMiddleware(request);
  };
};
gustaveWPM commented 11 months ago

Alright, I figured it out. I don't think it was actually related to your package, my bad.

For anyone wondering this is how I did it (following the tutorial above):

export const withI18n: MiddlewareFactory = (next: NextMiddleware) => {
  return async (request: NextRequest, _next: NextFetchEvent) => {
    await next(request, _next);
    return I18nMiddleware(request);
  };
};

Hello @sshmaxime What is the purpose of the await next(request, _next); line?

EDIT: lol, I've finally understood it on my depends. https://github.com/QuiiBz/next-international/issues/354

Tatamo commented 7 months ago

await next(request, _next); drops responses from other middlewares and this solution doesn't seem perfect.

Here is a simple example following the article:

import { NextMiddleware, NextRequest, NextResponse, NextFetchEvent } from "next/server";
import { createI18nMiddleware } from "next-international/middleware";

export const withHeaders = (next: NextMiddleware) => {
  return async (request: NextRequest, _next: NextFetchEvent) => {
    const res = await next(request, _next);
    if (res) {
      res.headers.set("x-test-values", "foo");
    }
    return res;
  };
};

const I18nMiddleware = createI18nMiddleware({
  locales: ["en", "ja"],
  defaultLocale: "en"
});

const withI18n = (next: NextMiddleware) => {
  return async (request: NextRequest, _next: NextFetchEvent) => {
    await next(request, _next);
    return I18nMiddleware(request);
  };
};

export async function middleware(request: NextRequest, event: NextFetchEvent) {
  const defaultMiddleware: NextMiddleware = () => NextResponse.next();

  return await withI18n(withHeaders(defaultMiddleware))(request, event);
}

Here withI18n drops response headers attached by another middleware.

@QuiiBz It is my understanding that this is next-international side problem it prevents to implement higher-order function to chain middlewares.

Tatamo commented 7 months ago

I have noticed an additional issue. The internal implementation of I18nMiddleware hardcodes NextResponse.next();, which prevents the addition of request headers as explained in Next.js documentation on setting headers.

This issue should be reopened.

gustaveWPM commented 7 months ago

Hey @Tatamo Thank you a lot for your feedback.

At the moment I'm not experiencing anything that's really stressing me out about this (I don't play much with headers). But I've already seen a few other issues mentioning problems at this level.

For instance:

Could you open a new issue with a small repo or just a small snippet of code that allows to reproduce the problem?

I think I have a code quite similar to the one you written in your current snippet.

I just cheated by putting the i18n middleware at the very end of the chain. And in the case of my use of only one another middleware (Next Auth), I haven't yet experienced too many unpleasant surprises.

But I feel it could explode in my project too, so I'd be interested in a dedicated issue.

Tatamo commented 7 months ago

Hello @gustaveWPM. Thank you for your suggestion. After reviewing the issue, I realized that the problems with request headers and response headers can be considered separately. Particularly, the issue with request headers seems hard to avoid.

I have created a new issue focusing specifically on the request header problem #341 .

On the other hand, as you mentioned, the response headers issue can be work-arounded by changing the order of middleware chaining. (Replace withI18n(withHeaders(defaultMiddleware)) -> withHeaders(withI18n(defaultMiddleware))) However, if similar implementations are present in other middlewares from different libraries... 😢

gustaveWPM commented 7 months ago

Hello @gustaveWPM. Thank you for your suggestion. After reviewing the issue, I realized that the problems with request headers and response headers can be considered separately. Particularly, the issue with request headers seems hard to avoid.

I have created a new issue focusing specifically on the request header problem #341 .

On the other hand, as you mentioned, the response headers issue can be work-arounded by changing the order of middleware chaining. (Replace withI18n(withHeaders(defaultMiddleware)) -> withHeaders(withI18n(defaultMiddleware))) However, if similar implementations are present in other middlewares from different libraries... 😢

Yes, I'm aware that my answer is not adequate. :/ Especially as it seems to me that headers should be able to be used outside the scope of middleware, i.e. even in server components. (EDIT: as you stated in your issue with your page.ts example)

I hope the issue will be reviewed soon.