i18nexus / next-i18n-router

Next.js App Router internationalized routing and locale detection.
MIT License
257 stars 19 forks source link

Added permanentRedirect option to i18nRouter #82

Closed samvelArm closed 3 months ago

samvelArm commented 3 months ago

There are situations when you could prefer to have permanent redirects with 301 status instead of temp redirects with 307/308 status.

I added optional prop for that.

i18nexus commented 3 months ago

Hi @samvelArm. I believe a permanent redirect for all redirects would have some problems.

Off the top of my head, this would cause issues with redirecting to a user-specified language. For example if your browser is set to English, you will be immediately permanently be redirected to /en on your first visit. If the user later changes their language to German via a UI button, they will still be redirected to /en the next time they visit the site at /.

I think this is an edge case and will cause problems and confusion since the developer really needs to know what they're doing. You can implement this in your middleware pretty easily if you really want this:

export function middleware(request: NextRequest) {
  const response = i18nRouter(request, i18nConfig);

  if (response.status === 307) {
    response.status = 301;
  }

  return response;
}
samvelArm commented 3 months ago

That is the option I need, what is the reason for implementing custom logic in the middleware when I am using lib for that?

This PR doesn't break anything, it's an optional change, and I didn't get the reason for declining that.

If someone doesn't need this change he can simply skip the param.

The permanent redirects are needed for SEO proposes as 301 doesn't get cached.

And I really didn't see the reason not including my changes in the library itself.

If the case it that I can implement this in the middleware, well I can implement in the middware the entire library, but the library doesn't give me the opportunity to select the status code of the redirect and cuts the next.js supported stuff.

I will have to remove the lib without this change.

i18nexus commented 3 months ago

I understand that it doesn't immediately break anything since you put it behind a config option. But unfortunately that's not a good enough reason by itself.

Every feature has to be maintained and supported for the lifespan of the library, and therefore has to be thoroughly discussed before adding it. In the future you may want to first create an issue to have a discussion about the best way to implement it, if at all.

Here are some more reasons why this wasn't merged:

  1. Permanent redirects do not make sense in the context of this library. This library uses cookies to tell the server which language to redirect to. A permanent redirect would completely override this functionality.
  2. A user will be unable to change their language. If a permanent redirect is sent from / to /de, then they change their language to es via a UI dropdown, they will still be redirected to /de.
  3. Temporary redirects are appropriate in this general situation as can be read here.
  4. After testing your code in our example project, changing languages and clicking between pages is currently causing me infinite redirect loops. It's worse when serverSetCookie is set to "never". After changing between serverSetCookie options, my browser is now in a very weird state where I cannot change languages and experiencing crashing due to infinite redirects.
  5. Even if we were to merge this PR, what happens if someone else decides they want to use a different status code other than 301 or 307, such as a 302 or 308? We would have to remove your permanentRedirect option and replace it with a redirectStatusCode config option. This would be a breaking change that would require a major version release.
  6. If anyone wants this functionality, it is only 2 lines of code to set it up using the code I provided you. I would not recommend it, but it's possible.