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

Normalize trailing slash #6421

Closed
wants to merge 23 commits into from
Closed

Conversation

Janpot
Copy link
Contributor

@Janpot Janpot commented Feb 23, 2019

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.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2019

Stats from current PR

zeit/next.js canary zeit/next.js refs/heads/canary
Build Duration 15.5s 16s
Base Page Size 1.2 kB 1.2 kB
Build Dir Size 385 kB 385 kB
Average Memory Usage 148 MB 148 MB
Max Memory Usage 207 MB 207 MB
Average CPU Usage 95.75% 95.79%
Max CPU Usage 111.11% 122.22%
node_modules Size 42.9 MB 42.9 MB

@Janpot
Copy link
Contributor Author

Janpot commented Mar 5, 2019

🤔 I should probably add a production test here as well, just like the /index/index.js PR.

@timneutkens
Copy link
Member

@Janpot yeah definitely, however I still think this needs to be discussed more, it's technically a breaking change to allow it I think 🤔

@timneutkens
Copy link
Member

@Janpot btw sorry for hijacking the PR branch update, I wanted to see if the new github action works 💯

@Janpot
Copy link
Contributor Author

Janpot commented Mar 5, 2019

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.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2019

Stats from current PR

Click to expand stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 16.3s 16.4s ⚠️ +0.93%
Client _app Size 2.67 kB 2.67 kB
Client _app gzip Size 1.12 kB 1.12 kB
Client _error Size 6.56 kB 6.56 kB
Client _error gzip Size 2.77 kB 2.77 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client main Size 20.2 kB 20.2 kB
Client main gzip Size 6.77 kB 6.77 kB
Client commons Size 190 kB 190 kB
Client commons gzip Size 61.2 kB 61.2 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Base Rendered Size 1.2 kB 1.2 kB
Build Dir Size 385 kB 385 kB ⚠️ +0%
Average Memory Usage 147 MB 148 MB ⚠️ +0.22%
Max Memory Usage 208 MB 208 MB ⚠️ +0.15%
Average CPU Usage 95.61% 94.22% -1.45%
Max CPU Usage 111.11% 111.11%
node_modules Size 42.8 MB 42.8 MB ⚠️ +0%

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2019

Stats from current PR

Click to expand stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 17.2s 16.6s -3.59%
Client _app Size 2.67 kB 2.67 kB
Client _app gzip Size 1.12 kB 1.12 kB
Client _error Size 6.56 kB 6.56 kB
Client _error gzip Size 2.77 kB 2.77 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client main Size 20.2 kB 20.2 kB
Client main gzip Size 6.77 kB 6.77 kB
Client commons Size 190 kB 190 kB
Client commons gzip Size 61.2 kB 61.2 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Base Rendered Size 1.2 kB 1.2 kB
Build Dir Size 385 kB 385 kB ⚠️ +0%
Average Memory Usage 146 MB 145 MB -0.62%
Max Memory Usage 206 MB 208 MB ⚠️ +0.79%
Average CPU Usage 94.11% 92.85% -1.34%
Max CPU Usage 111.11% 122.22% ⚠️ +10%
node_modules Size 42.8 MB 42.8 MB ⚠️ +0%

@Janpot
Copy link
Contributor Author

Janpot commented Mar 9, 2019

@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.

@timneutkens
Copy link
Member

This also solves #619 right?

@Janpot
Copy link
Contributor Author

Janpot commented Mar 9, 2019

I wasn't aware of that issue. I guess at least part of it is solved in this PR. This is the intended behavior?

@timneutkens
Copy link
Member

timneutkens commented Mar 9, 2019

Nah I think the behavior you implemented is fine, no need to make it more complex 👍

@Janpot
Copy link
Contributor Author

Janpot commented Mar 10, 2019

@timneutkens Just as a check, I went ahead and encoded the proposed behavior of #619 (comment) in some tests. These are my findings:

  1. several failures were fixed in this PR in a sense that now most of these links resolve to something instead of erroring out.
  2. client-side behavior: all cases seem to resolve to something. one case, where both ./folder.js and ./folder/index.js exist seems to always favor ./folder.js, even when linking to /folder/index.
  3. server-side there are still two cases that error out and thus are inconsistent with the client.

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.

@github-actions
Copy link
Contributor

Stats from current PR

Click to expand stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 13.7s 13.9s ⚠️ +1.74%
Client _app Size 2.67 kB 2.67 kB
Client _app gzip Size 1.13 kB 1.13 kB
Client _error Size 6.56 kB 6.56 kB
Client _error gzip Size 2.77 kB 2.77 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client main Size 20.2 kB 20.2 kB
Client main gzip Size 6.78 kB 6.78 kB ⚠️ +0.01%
Client commons Size 190 kB 190 kB
Client commons gzip Size 61 kB 61 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Base Rendered Size 1.2 kB 1.2 kB
Build Dir Size 316 kB 316 kB ⚠️ +0.01%
Average Memory Usage 134 MB 132 MB -1.35%
Max Memory Usage 172 MB 172 MB ⚠️ +0.21%
Average CPU Usage 94.38% 93.8% -0.61%
Max CPU Usage 111.11% 111.11%
node_modules Size 38.3 MB 38.3 MB ⚠️ +0.04%
Click to expand serverless stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 14.7s 15.1s ⚠️ +2.73%
Client _app Size 2.67 kB 2.67 kB
Client _app gzip Size 1.13 kB 1.13 kB
Client _error Size 6.56 kB 6.56 kB
Client _error gzip Size 2.77 kB 2.77 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client main Size 20.2 kB 20.2 kB
Client main gzip Size 6.78 kB 6.78 kB
Client commons Size 190 kB 190 kB
Client commons gzip Size 61 kB 61 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Serverless index Size 260 kB 260 kB ⚠️ +0.02%
Serverless index gzip Size 61.4 kB 61.4 kB 0%
Serverless _error Size 261 kB 261 kB ⚠️ +0.02%
Serverless _error gzip Size 61.4 kB 61.4 kB ⚠️ +0.15%
Build Dir Size 792 kB 792 kB ⚠️ +0.01%
Average Memory Usage 140 MB 138 MB -1.4%
Max Memory Usage 176 MB 205 MB ⚠️ +16.48%
Average CPU Usage 95.89% 94.97% -0.96%
Max CPU Usage 111.11% 111.11%
node_modules Size 38.3 MB 38.3 MB ⚠️ +0.04%

@github-actions
Copy link
Contributor

Stats from current PR

Click to expand stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 14.9s 15.5s ⚠️ +3.91%
Client _app Size 2.67 kB 2.67 kB
Client _app gzip Size 1.13 kB 1.13 kB
Client _error Size 6.56 kB 6.56 kB
Client _error gzip Size 2.77 kB 2.77 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client main Size 20.2 kB 20.2 kB
Client main gzip Size 6.78 kB 6.78 kB
Client commons Size 190 kB 190 kB
Client commons gzip Size 61 kB 61 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Base Rendered Size 1.2 kB 1.2 kB
Build Dir Size 316 kB 316 kB ⚠️ +0.01%
Average Memory Usage 132 MB 134 MB ⚠️ +1.07%
Max Memory Usage 169 MB 172 MB ⚠️ +1.75%
Average CPU Usage 93.18% 94.2% ⚠️ +1.09%
Max CPU Usage 111.11% 122.22% ⚠️ +10%
node_modules Size 38.3 MB 38.3 MB ⚠️ +0.04%
Click to expand serverless stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 15.6s 15.8s ⚠️ +1.45%
Client _app Size 2.67 kB 2.67 kB
Client _app gzip Size 1.13 kB 1.13 kB
Client _error Size 6.56 kB 6.56 kB
Client _error gzip Size 2.77 kB 2.77 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client main Size 20.2 kB 20.2 kB
Client main gzip Size 6.78 kB 6.78 kB
Client commons Size 190 kB 190 kB
Client commons gzip Size 61 kB 61 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Serverless index Size 260 kB 260 kB ⚠️ +0.02%
Serverless index gzip Size 61.4 kB 61.4 kB ⚠️ +0%
Serverless _error Size 261 kB 261 kB ⚠️ +0.02%
Serverless _error gzip Size 61.5 kB 61.5 kB ⚠️ +0.03%
Build Dir Size 792 kB 792 kB ⚠️ +0.01%
Average Memory Usage 138 MB 139 MB ⚠️ +0.35%
Max Memory Usage 209 MB 206 MB -1.39%
Average CPU Usage 95.11% 95.62% ⚠️ +0.54%
Max CPU Usage 111.11% 122.22% ⚠️ +10%
node_modules Size 38.3 MB 38.3 MB ⚠️ +0.04%

@Janpot
Copy link
Contributor Author

Janpot commented Mar 10, 2019

Added the same tests for production.

development:

    page resolution
      client side
        ✓ should resolve to js file when no slash (2021ms)
        ✕ should resolve to folder index when slash (1014ms)
        ✕ should resolve to folder index when index (950ms)
        ✓ should resolve to folder when no slash and no js file (1488ms)
        ✓ should resolve to folder index when slash and no js file (999ms)
        ✓ should resolve to folder index when index and no js file (1011ms)
      server side 
        ✓ should resolve to js file when no slash (890ms)
        ✕ should resolve folder index when slash (857ms)
        ✕ should resolve folder index when index (945ms)
        ✓ should resolve to js file when no slash and no js file (770ms)
        ✓ should resolve to js file when slash and no js file (801ms)
        ✓ should resolve folder index when index and no js file (871ms)

production:

    page resolution
      client side
        ✕ should resolve to js file when no slash (2551ms)
        ✓ should resolve to folder index when slash (839ms)
        ✓ should resolve to folder index when index (872ms)
        ✓ should resolve to folder when no slash and no js file (727ms)
        ✓ should resolve to folder index when slash and no js file (753ms)
        ✓ should resolve to folder index when index and no js file (732ms)
      server side 
        ✕ should resolve to js file when no slash (709ms)
        ✓ should resolve folder index when slash (691ms)
        ✓ should resolve folder index when index (731ms)
        ✓ should resolve to js file when no slash and no js file (652ms)
        ✓ should resolve to js file when slash and no js file (655ms)
        ✓ should resolve folder index when index and no js file (691ms)

So half of the production tests are in line with node resolution, and half of development tests are. Unfortunately it's different halves 😄.

@github-actions
Copy link
Contributor

Stats from current PR

Click to expand stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 13.4s 13s -2.32%
Client _app Size 2.67 kB 2.67 kB
Client _app gzip Size 1.13 kB 1.13 kB
Client _error Size 6.56 kB 6.56 kB
Client _error gzip Size 2.77 kB 2.77 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client main Size 20.2 kB 20.2 kB
Client main gzip Size 6.78 kB 6.78 kB
Client commons Size 190 kB 190 kB
Client commons gzip Size 61 kB 61 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Base Rendered Size 1.2 kB 1.2 kB
Build Dir Size 316 kB 316 kB ⚠️ +0.01%
Average Memory Usage 132 MB 132 MB -0.1%
Max Memory Usage 172 MB 169 MB -1.77%
Average CPU Usage 91.73% 92.52% ⚠️ +0.86%
Max CPU Usage 111.11% 111.11%
node_modules Size 38.3 MB 38.3 MB ⚠️ +0%
Click to expand serverless stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 14.1s 14.1s ⚠️ +0.3%
Client _app Size 2.67 kB 2.67 kB
Client _app gzip Size 1.13 kB 1.13 kB
Client _error Size 6.56 kB 6.56 kB
Client _error gzip Size 2.77 kB 2.77 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client main Size 20.2 kB 20.2 kB
Client main gzip Size 6.78 kB 6.78 kB
Client commons Size 190 kB 190 kB
Client commons gzip Size 61 kB 61 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Serverless index Size 260 kB 260 kB ⚠️ +0.02%
Serverless index gzip Size 61.5 kB 61.4 kB -0.1%
Serverless _error Size 261 kB 261 kB ⚠️ +0.02%
Serverless _error gzip Size 61.4 kB 61.4 kB ⚠️ +0.05%
Build Dir Size 792 kB 792 kB ⚠️ +0.01%
Average Memory Usage 139 MB 141 MB ⚠️ +1.56%
Max Memory Usage 175 MB 177 MB ⚠️ +1.31%
Average CPU Usage 93.25% 94.58% ⚠️ +1.43%
Max CPU Usage 111.11% 111.11%
node_modules Size 38.3 MB 38.3 MB ⚠️ +0%

@github-actions
Copy link
Contributor

Stats from current PR

Click to expand stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 12.4s 13.1s ⚠️ +5.49%
Client _app Size 3.02 kB 3.02 kB
Client _app gzip Size 1.27 kB 1.27 kB
Client _error Size 7.89 kB 7.89 kB
Client _error gzip Size 3.15 kB 3.15 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client pages/link Size 3.32 kB 3.32 kB
Client pages/link gzip Size 1.5 kB 1.5 kB
Client pages/routerDirect Size 411 B 411 B
Client pages/routerDirect gzip Size 297 B 297 B
Client pages/withRouter Size 393 B 393 B
Client pages/withRouter gzip Size 281 B 281 B
Client main Size 21.9 kB 21.9 kB
Client main gzip Size 7.26 kB 7.26 kB ⚠️ +0.01%
Client commons Size 188 kB 188 kB
Client commons gzip Size 60.7 kB 60.7 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Base Rendered Size 1.2 kB 1.2 kB
Build Dir Size 459 kB 459 kB ⚠️ +0%
Average Memory Usage 129 MB 129 MB -0.41%
Max Memory Usage 165 MB 170 MB ⚠️ +2.79%
Average CPU Usage 93.76% 93.13% -0.67%
Max CPU Usage 111.11% 110% -1%
node_modules Size 42.8 MB 42.8 MB ⚠️ +0%
Click to expand serverless stats
zeit/next.js canary Janpot/next.js normalize-trailing-slash Change
Build Duration 16.6s 16.6s -0.1%
Client _app Size 3.02 kB 3.02 kB
Client _app gzip Size 1.27 kB 1.27 kB
Client _error Size 7.89 kB 7.89 kB
Client _error gzip Size 3.15 kB 3.15 kB
Client pages/index Size 288 B 288 B
Client pages/index gzip Size 222 B 222 B
Client pages/link Size 3.32 kB 3.32 kB
Client pages/link gzip Size 1.5 kB 1.5 kB
Client pages/routerDirect Size 411 B 411 B
Client pages/routerDirect gzip Size 297 B 297 B
Client pages/withRouter Size 393 B 393 B
Client pages/withRouter gzip Size 281 B 281 B
Client main Size 21.9 kB 21.9 kB
Client main gzip Size 7.26 kB 7.26 kB
Client commons Size 188 kB 188 kB
Client commons gzip Size 60.7 kB 60.7 kB
Client webpack Size 1.49 kB 1.49 kB
Client webpack gzip Size 769 B 769 B
Serverless pages/link Size 264 kB 264 kB ⚠️ +0.02%
Serverless pages/link gzip Size 62.6 kB 62.6 kB ⚠️ +0.05%
Serverless pages/index Size 259 kB 259 kB ⚠️ +0.02%
Serverless pages/index gzip Size 61.5 kB 61.5 kB ⚠️ +0.02%
Serverless pages/_error Size 260 kB 260 kB ⚠️ +0.02%
Serverless pages/_error gzip Size 61.4 kB 61.4 kB ⚠️ +0.02%
Serverless pages/routerDirect Size 259 kB 259 kB ⚠️ +0.02%
Serverless pages/routerDirect gzip Size 61.4 kB 61.4 kB ⚠️ +0.02%
Serverless pages/withRouter Size 259 kB 259 kB ⚠️ +0.02%
Serverless pages/withRouter gzip Size 61.5 kB 61.5 kB ⚠️ +0.02%
Build Dir Size 1.58 MB 1.58 MB ⚠️ +0.01%
Average Memory Usage 143 MB 144 MB ⚠️ +0.38%
Max Memory Usage 209 MB 209 MB -0.01%
Average CPU Usage 95.14% 95.89% ⚠️ +0.79%
Max CPU Usage 111.11% 111.11%
node_modules Size 42.8 MB 42.8 MB ⚠️ +0%

@Janpot
Copy link
Contributor Author

Janpot commented Mar 13, 2019

@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.

@timneutkens
Copy link
Member

Probably good to note that

Nah I think the behavior you implemented is fine, no need to make it more complex 👍

meant I didn't want to replicate Node.js require behavior.

@Janpot
Copy link
Contributor Author

Janpot commented Mar 13, 2019

but it makes sense to define "some" behavior for these edge cases and make them consistent across server/client and development/production, right?

@timneutkens
Copy link
Member

Yep!

@Janpot
Copy link
Contributor Author

Janpot commented Mar 13, 2019

Ok, so I guess it's a matter of filling in the blanks then (url => file on disk)


pages/
  subfolder.js
  1. https://example.com/subfolder => /pages/subfolder.js
  2. https://example.com/subfolder/ => /pages/subfolder.js
  3. https://example.com/subfolder/index => 404

pages/
  subfolder/
    index.js
  subfolder.js

This would result in a build error


pages/
  subfolder/
    index.js
  1. https://example.com/subfolder => /pages/subfolder/index.js
  2. https://example.com/subfolder/ => /pages/subfolder/index.js
  3. https://example.com/subfolder/index => 404

pages/
  index/
    index.js
  1. https://example.com/ => 404
  2. https://example.com/index => /pages/index/index.js
  3. https://example.com/index/ => /pages/index/index.js
  4. https://example.com/index/index => 404

pages/
  index.js
  1. https://example.com/ => /pages/index.js
  2. https://example.com/index => 404

pages/
  index/
    index.js
  index.js

This would be the only exception that duplicates file name/folder name that doesn't throw build errors

  1. https://example.com/ => /pages/index.js
  2. https://example.com/index => /pages/index/index.js
  3. https://example.com/index/ => /pages/index/index.js
  4. https://example.com/index/index => 404

@timneutkens
Copy link
Member

So imo we should for "one or the other" and not allow both. Meaning:

pages/index.js => /
pages/index/index.js => /

But don't allow both to exist

pages/about.js => /about / /about/
pages/about/index.js => /about / /about/

But don't allow both to exist. Also don't allow /about/index.

Does that make sense?

@Janpot
Copy link
Contributor Author

Janpot commented Mar 13, 2019

So what you say is that the

/pages
  /subfolder
    /index.js
  /subfolder.js

situation should throw a build error? That would eliminate 4. 5. and 6.
and then for 2. and 3.

  1. /subfolder/ => /pages/subfolder.js
  2. /subfolder/index => 404

That makes sense to me. But it's going to be a breaking change.

I updated my previous comment

@Janpot
Copy link
Contributor Author

Janpot commented Mar 20, 2019

This is my proposal. Will close until there is any kind of consensus on what behavior is desired.

@Janpot Janpot closed this Mar 20, 2019
@ellej16
Copy link

ellej16 commented May 6, 2019

@Janpot 's proposal sounds good enough. Are there any other concerns that blocks the merging of this feature / fix @timneutkens ?

We could include a strict prop to do the one or the other scenario, but still give the devs flexibility on which they want?

@kjs3
Copy link
Contributor

kjs3 commented Oct 1, 2019

Seems like this should be merged. Is there something specific holding this back? Is there a lack of support or consensus on this behavior?

@Janpot
Copy link
Contributor Author

Janpot commented May 18, 2020

Ok, nevermind, this is too far behind.

@Janpot Janpot closed this May 18, 2020
@Janpot Janpot deleted the normalize-trailing-slash branch May 18, 2020 16:38
@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 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