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

skipTrailingSlashRedirect option is not being applied #54984

Closed
1 task done
blauakqa opened this issue Sep 4, 2023 · 5 comments · Fixed by #55067
Closed
1 task done

skipTrailingSlashRedirect option is not being applied #54984

blauakqa opened this issue Sep 4, 2023 · 5 comments · Fixed by #55067
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@blauakqa
Copy link

blauakqa commented Sep 4, 2023

Link to the code that reproduces this issue or a replay of the bug

https://github.com/blauakqa/nextjs-trailing-slash-issue

To Reproduce

To replicate:

  • npm install
  • npm run dev
  • open the browser and go to http://localhost:300
  • check the link (link-with-slash) and the trailing slash is removed

Current vs. Expected behavior

Expect the rendered url /link-with-slash/ to have a trailing slash under the Links with trailing slash header
Actual result is the trailing slash is removed

Verify canary release

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

Provide environment information

Operating System:
      Platform: darwin
      Arch: arm64
      Version: Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:20 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6000
    Binaries:
      Node: 18.12.1
      npm: 8.19.2
      Yarn: 1.22.15
      pnpm: 6.11.0
    Relevant Packages:
      next: 13.4.20-canary.15
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: N/A
    Next.js Config:
      output: N/A

Which area(s) are affected? (Select all that apply)

Routing (next/router, next/navigation, next/link)

Additional context

We have skipTrailingSlashRedirect set to true.

If we set breakpoints, we can see that on line 294 of webpack-config.js, config.skipTrailingSlashRedirect is true.

image

However, when we hit line 14 of normalize-trailing-slash.js, process.env.__NEXT_MANUAL_TRAILING_SLASH is undefined in the normalizePathTrailingSlash function

image

If we manually set __NEXT_MANUAL_TRAILING_SLASH to true via a .env file, it works as expected.

The supplied example has two urls

  • /link-with-slash/
  • /link-no-slash

When you run the example, have noticed the following when normalizePathTrailingSlash function is executed

  • First execution, path parameter is /link-with-slash/ and the value of process.env.__NEXT_MANUAL_TRAILING_SLASH is undefined
  • Second execution, path parameter is /link-with-slash and the value of process.env.__NEXT_MANUAL_TRAILING_SLASH is true - trailing slash is already removed

Have tried it on the following NextJS versions and the issue still occurs locally and on Vercel

  • 13.4.4
  • 13.4.19
  • canary (v13.4.20-canary.15)

Cheers

NEXT-1599

@blauakqa blauakqa added the bug Issue was opened via the bug report template. label Sep 4, 2023
@github-actions github-actions bot added the Navigation Related to Next.js linking (e.g., <Link>) and navigation. label Sep 4, 2023
@williamli
Copy link
Contributor

@blauakqa I think trailingSlash and skipTrailingSlashRedirect are working as intended and they have just been a little misunderstood.

trailingSlash

By default Next.js will redirect urls with trailing slashes to their counterpart without a trailing slash. For example /about/ will redirect to /about. You can configure this behavior to act the opposite way, where urls without trailing slashes are redirected to their counterparts with trailing slashes.

When enabled:

  1. <Link href="/about">About</Link> will take visitors to /about/
  2. Request to /about will get redirected to /about/

When disabled:

  1. <Link href="/about">About</Link> will take visitors to /about
  2. Request to /about/ will get redirected to /about

With skipTrailingSlashRedirect set to true, the redirects mentioned in point 2 above will not occur:

  1. Request to /about/ will render the about page under /about/
  2. Request to /about will render the about page under /about

Demo

I forked your repo, tested with Next.js 13.4.19 and canary (v13.4.20-canary.18) and everything seems fine.

This is what I get running on my local:
http://localhost:3000/link-with-slash/
CleanShot 2023-09-06 at 14 33 57
http://localhost:3000/link-with-slash
CleanShot 2023-09-06 at 15 10 45

deployed v13.4.19:
https://154699-nextjs-trailing-slash-issue-git-13-4-9.preview.vercel-support.app/link-with-slash/
CleanShot 2023-09-06 at 15 14 38
https://154699-nextjs-trailing-slash-issue-git-13-4-9.preview.vercel-support.app/link-with-slash
CleanShot 2023-09-06 at 15 14 53

@blauakqa
Copy link
Author

blauakqa commented Sep 6, 2023

Thanks William,

To clarify are you implying that skipTrailingSlashRedirect property is only for after the user clicks the link?
Originally thought that but after checking NextJS code we thought it would also preserve the URL as supplied before the users clicks the link. Seeing the behaviour we're seeing the former makes sense

If you go to the homepage from the repo, we wish to preserve the URL as is:

image

If that’s not the case, apart from patching the code is there a way to achieve this desired behaviour of preserving the URL?

Cheers,
Ben

@williamli
Copy link
Contributor

williamli commented Sep 6, 2023

To summarise:

  1. After click on link behaviour is correct
  2. Direct URL access behaviour is correct (when an external link ask for /something and /something-else/ with skipTrailingSlashRedirect set to true, take visitor to /something and /something-else/ respectively, without redirect)
  3. The javascript renderings of links are correct
  4. The raw HTML output of links are not correct (ie, if you access the raw HTML using curl https://154699-nextjs-trailing-slash-issue-git-13-4-9.preview.vercel-support.app/, the slashes are removed).

skipTrailingSlashRedirect , as its name suggests, only affects the slash-redirecting behaviour of Next.js, so what you are after is either a bug or a feature request of behaviour that should not be controlled by the skipTrailingSlashRedirect settings. I will notify the Next.js team and see what can be done.

@balazsorban44 balazsorban44 added the linear: next Confirmed issue that is tracked by the Next.js team. label Sep 6, 2023
@balazsorban44
Copy link
Member

After some bisecting, I could trace this to a regression between 13.1.7-canary.1 and 13.1.7-canary.2. The behavior is actually correct, but a link with a trailing slash is incorrectly rendered without a trailing slash for the first time as observed.

balazsorban44 added a commit that referenced this issue Sep 6, 2023
@kodiakhq kodiakhq bot closed this as completed in #55067 Sep 6, 2023
kodiakhq bot pushed a commit that referenced this issue Sep 6, 2023
This moves `resolve-href` into `next/src/client` to make sure that when it calls `normalizeTrailingSlash`, that function has access to `process.env.__NEXT_MANUAL_TRAILING_SLASH` (inlined via `DefinePlugin`).

Closes NEXT-1599
Fixes #54984
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. 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 Sep 21, 2023
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. linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants