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

Middleware rewrites do not automatically handle i18n #36563

Closed
1 task done
joshuabaker opened this issue Apr 29, 2022 · 6 comments
Closed
1 task done

Middleware rewrites do not automatically handle i18n #36563

joshuabaker opened this issue Apr 29, 2022 · 6 comments
Labels
bug Issue was opened via the bug report template. Middleware Related to Next.js Middleware

Comments

@joshuabaker
Copy link
Contributor

joshuabaker commented Apr 29, 2022

Verify canary release

  • I verified that the issue exists in Next.js canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 21.4.0: Fri Mar 18 00:46:32 PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T6000
Binaries:
  Node: 16.13.0
  npm: 8.1.0
  Yarn: 1.22.17
  pnpm: 6.30.1
Relevant packages:
  next: 12.1.6-canary.14
  react: 18.1.0
  react-dom: 18.1.0

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

Vercel

Describe the Bug

Server side renders are return 404s for paths adjusted by rewrites in our _middleware.js. Client side routing worked fine for this paths, which meant these 404s weren’t immediately apparent in QA.

This took quite a while to debug this; When i18n is provided in next.config.js middleware rewrites break.

Expected Behavior

Next.js generally does an amazing job of reconciling the default locale automatically. It’d be great if this would be extended to the middleware as well (i.e. prepend the pathname with the default locale, if is doesn’t start with a locale).

It’s also possible that this is by design. If so, I would suggest calling this out clearly in the documentation (i.e. Caveats).

To Reproduce

Clone this repo and deploy to Vercel.

Both host1.testingwebsite.co and host2.testingwebsite.co that are configured in the middleware are pointing to cname.vercel-dns.com for your convenience.

@joshuabaker joshuabaker added the bug Issue was opened via the bug report template. label Apr 29, 2022
@joshuabaker
Copy link
Contributor Author

Possible duplicates or variants of the same underlying issue: #33044, #36117

@joshuabaker
Copy link
Contributor Author

@javivelasco From your comment on #33044 I would assume that this is not something you intend to support automatically.

Please could I suggest a note in the documentation to warn developers about this. There’s a lot of people that see this as a bug due to the ‘magic’ nature of locale routing without middleware.

@javivelasco
Copy link
Member

@joshuabaker I was about to reproduce this same issue to see if it is the same source. From the code you referenced this seems to be slightly different because you are indeed using NextURL to perform the rewrite and therefore the locale should be preserved.

What we can't support directly is inferring if your redirection is intended to preserve the locale or not. There is a plethora of use cases when it comes to i18n so I believe the most effective way to support all cases is to be explicit by providing the full URL where you want to rewrite.

Indeed it makes sense to mention in the documentation that the user must provide the full URL and that Next.js will give you the utils to make it a painless experience. Like I said, in your example the redirection should just work fine so I'm gonna try to reproduce and get back into this issue with what I see.

@joshuabaker
Copy link
Contributor Author

Thanks, @javivelasco. Totally understand.

Next.js will give you the utils to make it a painless experience

Perhaps as part of the NextUrl the locale might be included, assuming that doesn’t happen much later in the pipeline?

N.B. Those domains I provided may not work now. Sorry if they don’t.

@javivelasco
Copy link
Member

@joshuabaker I've just tried to reproduce with the most recent canary but I didn't use the hostnames but instead used a query parameter to decide how to rewrite. In practical terms it works exactly in the same way if I'm not missing anything. To update to the latest canary you need to move your middleware file from pages/_middleware.js to /middleware.js in the root folder. All APIs you are using are compatible with the latest version so my testing code looks like this:

import { NextResponse } from "next/server";

export default function middleware(req) {
  const url = req.nextUrl.clone();
  const siteId = req.nextUrl.searchParams.get("site") || "0";
  if (!["0", "1", "2"].includes(siteId)) {
    return new Response(null, { status: 404 });
  }

  if (
    url.pathname.includes(".") || // exclude file paths (e.g. public folder)
    url.pathname.startsWith("/api") || // exclude all API routes
    req.headers.has("x-prerender-revalidate") // exclude revalidate requests
  ) {
    return undefined; // Do nothing
  }

  if (url.pathname.startsWith(`/_sites`) || !siteId) {
    return new Response(null, { status: 404 });
  }

  if (url.pathname === "/blog") {
    url.pathname += "/page/1";
  }

  url.pathname = `/_sites/${siteId}${url.pathname}`;
  return NextResponse.rewrite(url);
}

First I tested the app exactly as in your repository and indeed visiting the root page you get a 404. I did reproduce it. After updating I deployed again and it all works as expected. If you change the query parameter to be a different site rewrites will work just fine.

About NextURL indeed it does. For example, if you have a basePath say /root and support es locale and you have a request to https://test.vercel.app/root/es/about then NextURL will be:

request.nextUrl.locale // 'es'
request.nextUrl.basePath // '/root'
request.nextUrl.pathname // '/about'
String(request.nextUrl) // https://test.vercel.app/root/es/about

Since it all seems to be working fine, I'm closing this issue!
Thanks for reporting and for the feedback 🙏

@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. Middleware Related to Next.js Middleware
Projects
None yet
Development

No branches or pull requests

3 participants