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

Rewrites bug when using with basePath and public directory #16332

Closed
giwan-dev opened this issue Aug 19, 2020 · 4 comments · Fixed by #16356
Closed

Rewrites bug when using with basePath and public directory #16332

giwan-dev opened this issue Aug 19, 2020 · 4 comments · Fixed by #16356
Assignees
Milestone

Comments

@giwan-dev
Copy link

Bug report

Describe the bug

Next.js cannot rewrites URL when using basePath option and having public directory.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. Clone the reproduction repo
  2. Install dependencies, build next server, and run it.
npm install
npm run build
npm run start
  1. GET the url what should be rewritten.
curl 'http://localhost:3000/todos'
  1. See error

Expected behavior

$ curl 'http://localhost:3000/todos'
HTTP/1.1 200 OK
...

System information

  • OS: macOS
  • Version of Next.js: 9.5.2
  • Version of Node.js: v12.18.0

Additional context

Add any other context about the problem here.

@inbeom
Copy link

inbeom commented Aug 19, 2020

Next.js dev server behaves as expected. After running npm run dev:

$ curl -I http://localhost:3000/todos
HTTP/1.1 200 OK
date: Wed, 19 Aug 2020 02:56:51 GMT
content-type: application/json; charset=utf-8
connection: close
...

It fails only if the server is running in non-development mode.

@inbeom
Copy link

inbeom commented Aug 19, 2020

After some investigation, I found the current matching order of Routes always puts public routes which is in fsRoutes before rewrite routes if ./public directory is present.

From this if branch for early routing failure added from #15041 lets next-server to respond with 404 to every request not having basePath as its path prefix for public routes. It assumes all of the non-custom routes with lower precedence than the fs route requires basePath to be present, while for rewrite routes it might not be true.

Although I am not fully aware of the context of this decision, but I suppose the assumption of public routes and the early routing failure conflicts with each other. We might:

  • continue regardless of requireBasePath: This runs fine on my environment. But I think there might be problems regarding the final catchAllRoute, which would create false positive matches. (Or not? 😵)
  • Pass publicRoutes and the rest of fsRoutes separately down to Router, and put publicRoutes after rewrites: It seems to be a safer choice, but it does not fully reflect the priority of routes order introduced from Update next-server routes order for expected priority #10326

Which direction should we take further?

@inbeom
Copy link

inbeom commented Aug 19, 2020

Opened a PR with the first solution. Hope this helps. Thanks!

@ijjk ijjk added kind: bug and removed point: 2 labels Aug 19, 2020
@ijjk ijjk self-assigned this Aug 19, 2020
@kodiakhq kodiakhq bot closed this as completed in #16356 Aug 19, 2020
kodiakhq bot pushed a commit that referenced this issue Aug 19, 2020
This corrects the basePath being required check for filesystem routes to not consider the public folder catch-all route since it always matches even if the public file isn't present and instead moves the basePath check inside of the public-folder catch-all. Tests already exist that catch this by adding a public folder to the existing `basepath` test suite

Fixes: #16332
Closes: #16350
@Timer Timer added this to the iteration 7 milestone Aug 19, 2020
m-lautenbach pushed a commit to m-lautenbach/next.js that referenced this issue Aug 20, 2020
This corrects the basePath being required check for filesystem routes to not consider the public folder catch-all route since it always matches even if the public file isn't present and instead moves the basePath check inside of the public-folder catch-all. Tests already exist that catch this by adding a public folder to the existing `basepath` test suite

Fixes: vercel#16332
Closes: vercel#16350
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants