-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
Normalize trailing slash #6421
Normalize trailing slash #6421
Conversation
…next.js into normalize-trailing-slash
Stats from current PR
|
🤔 I should probably add a production test here as well, just like the |
@Janpot yeah definitely, however I still think this needs to be discussed more, it's technically a breaking change to allow it I think 🤔 |
@Janpot btw sorry for hijacking the PR branch update, I wanted to see if the new github action works 💯 |
IMO, the only scenario where this really is a breaking change is when you were relying on client and server behaving differently, which is kind of silly. |
Stats from current PRClick to expand stats
|
Stats from current PRClick to expand stats
|
@timneutkens Added production tests. Didn't have to change any test in production. So I guess the only "breaking change" here is in development mode, on the server side. Not super breaking I'd say. |
This also solves #619 right? |
I wasn't aware of that issue. I guess at least part of it is solved in this PR. This is the intended behavior? |
Nah I think the behavior you implemented is fine, no need to make it more complex 👍 |
@timneutkens Just as a check, I went ahead and encoded the proposed behavior of #619 (comment) in some tests. These are my findings:
I'd say, the most important thing is to make client and server behavior consistent, even if it differs from node resolution. Although parity with node resolution would be quite nice as well. |
Stats from current PRClick to expand stats
Click to expand serverless stats
|
Stats from current PRClick to expand stats
Click to expand serverless stats
|
Added the same tests for production. development:
production:
So half of the production tests are in line with node resolution, and half of development tests are. Unfortunately it's different halves 😄. |
Stats from current PRClick to expand stats
Click to expand serverless stats
|
Stats from current PRClick to expand stats
Click to expand serverless stats
|
@dav-is FYI: I added these failing tests but haven't had time to figure out a fix for them yet. My time to work on this is very sparse so feel free to take a stab at it if you want. These last 12 tests I added are meant to replicate how node.js require works in the next page resolution. Each piece of functionality seems to works somewhere (dev/prod) but nowhere does it all work. edit Next time I'm working this, I'll probably start over with these new tests. And trying to start by making production work properly first. |
Probably good to note that
meant I didn't want to replicate Node.js require behavior. |
but it makes sense to define "some" behavior for these edge cases and make them consistent across server/client and development/production, right? |
Yep! |
Ok, so I guess it's a matter of filling in the blanks then (
This would result in a build error
This would be the only exception that duplicates file name/folder name that doesn't throw build errors
|
So imo we should for "one or the other" and not allow both. Meaning:
But don't allow both to exist
But don't allow both to exist. Also don't allow Does that make sense? |
So what you say is that the
situation should throw a build error? That would eliminate 4. 5. and 6.
That makes sense to me. But it's going to be a breaking change. I updated my previous comment |
This is my proposal. Will close until there is any kind of consensus on what behavior is desired. |
@Janpot 's proposal sounds good enough. Are there any other concerns that blocks the merging of this feature / fix @timneutkens ? We could include a |
Seems like this should be merged. Is there something specific holding this back? Is there a lack of support or consensus on this behavior? |
Ok, nevermind, this is too far behind. |
fix #5214
fix #6277
linking to urls with trailing slashes works on the client-side. Directly navigating to them resulted in errors though. Both in production and dev mode. removing the trailing slash in a few places fixes the issue.
I had to change some tests since the error behavior serverside was "expected" according to the tests, the client side wasn't covered by tests. For now I changed the behavior to tolerate trailing slashes and basically treat them the same as trailing
/index
. I added a test to cover client-side and updated the serverside tests.Alternatively the behavior could be restricted (although one could ask the question to restrict the
/index
case as well). in that case the serverside tests need to be reverted and the client test needs to be changed to return an error instead. Though IMO, the way I implemented it now makes most sense and is most consistent.