Trailing slash redirect broken when using path prefix

patrickdemers6 commented 4 months ago

Preliminary Checks

[X] This issue is not a duplicate. Before opening a new issue, please search existing issues: https://github.com/gatsbyjs/gatsby/issues
[X] This issue is not a question, feature request, RFC, or anything other than a bug report directly related to Gatsby. Please post those things in GitHub Discussions: https://github.com/gatsbyjs/gatsby/discussions

Description

A project using pathPrefix and trailingSlash does not redirect properly.

When a page is loaded and requires a redirect due to the slash, the redirect does not take pathPrefix into account.

Reproduction Link

https://github.com/patrickdemers6/gatsby-slash-redirect-path-prefix-bug

Steps to Reproduce

Set pathPrefix to anything.
Set trailingSlash to never.
Run with gatsby build --prefix-paths and gatsby serve --prefix-paths
Load a page with trailing slash.

Example: pathPrefix = /prefix

Expected Result

The redirect removes just the trailing slash.

Expected /prefix/hello/ => /prefix/hello

Actual Result

The redirect removes the slash and the pathPrefix.

Actual /prefix/hello/ => /hello

Environment

System:
    OS: macOS 14.5
    CPU: (12) arm64 Apple M2 Pro
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.17.0 - ~/.nvm/versions/node/v18.17.0/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v18.17.0/bin/yarn
    npm: 9.6.7 - ~/.nvm/versions/node/v18.17.0/bin/npm
  Browsers:
    Chrome: 126.0.6478.127
    Safari: 17.5
  npmPackages:
    gatsby: ^5.14.0-next.4 => 5.14.0-next.4
  npmGlobalPackages:
    gatsby-cli: 5.13.3

Config Flags

No response

rosszurowski commented 1 week ago

I was running into this too. I noticed that Gatsby seems to be dead and while in the long run, I expect to move away from Gatsby, but in the short-term, I needed a fix.

Issue

It seems like the source of the bug is in gatsby-link, where several code paths don't call withPrefix. I cloned the gatsby repo, and modified the gatsby-link package to add the appropriate lines. (Confusingly, this file seems to have an isAbsolutePath function which returns true for relative paths).

diff --git a/packages/gatsby-link/src/rewrite-link-path.js b/packages/gatsby-link/src/rewrite-link-path.js
index e1b2b49671..6668b76c9b 100644
--- a/packages/gatsby-link/src/rewrite-link-path.js
+++ b/packages/gatsby-link/src/rewrite-link-path.js
@@ -13,25 +13,26 @@ const getGlobalTrailingSlash = () =>
 function applyTrailingSlashOptionOnPathnameOnly(path, option) {
   const { pathname, search, hash } = parsePath(path)
   const output = applyTrailingSlashOption(pathname, option)

   return `${output}${search}${hash}`
 }

 function absolutify(path, current) {
   // If it's already absolute, return as-is
   if (isAbsolutePath(path)) {
-    return path
+    return withPrefix(path)
   }

+  const prefixed = withPrefix(path)
   const option = getGlobalTrailingSlash()
-  const absolutePath = resolve(path, current)
+  const absolutePath = resolve(prefixed, current)

   if (option === `always` || option === `never`) {
     return applyTrailingSlashOptionOnPathnameOnly(absolutePath, option)
   }

   return absolutePath
 }

 function applyPrefix(path) {
   const prefixed = withPrefix(path)

Fixing It

The site I'm working on uses patch-package to apply the above change to Gatsby link. To generate this yourself, you can clone the repo, apply the above change, build, and then generate a patch from that.

If you'd rather re-use my work, you can copy the patch file in this gist to patches/gatsby-link+5.10.0.patch in your repository.

I haven't tested this too thoroughly, but it seems to handle all the cases I've thrown at it so far.

serhalp commented 1 week ago

Thank you for sharing your solution with the community @rosszurowski!

Though we aren't actively investing in new features or low-priority bug fixes, Gatsby is still maintained.

If you open a PR with your bug fix, we'll take a look and get it merged so everyone can benefit! Thanks 🙂

rosszurowski commented 1 week ago

Hi @serhalp — good to know! I didn't see an official response in that thread, so assumed it was accurate. I'll gladly open a PR with the fix.

serhalp commented 1 week ago

Awesome, thank you!

For the record, there is a response here and a blog post here.

gatsbyjs / gatsby