gatsbyjs / gatsby

The best React-based framework with performance, scalability and security built in.
https://www.gatsbyjs.com
MIT License
55.21k stars 10.33k forks source link

Unwanted trailing slash on prefixPath's root #23828

Closed hlolli closed 4 years ago

hlolli commented 4 years ago

Description

I'm working for a company which uses gatsby for building multilanguage website.

For both SEO reasons and aesthetics, we just don't want to see a trailing slash on the homepage, which in our case is the root of the prefixPath.

www.mydomain.com/english

would always become

www.mydomain.com/english/

I know that gatsby build --prefix-paths and gatsby serve will cause an express 301, just to be clear, I'm not refering to that, I see gatsby serve as a devtool and don't care about a slash there. We use nginx to redirect trailing slash to urls without trailing slashes, but gatsby (or reach router, not sure) will still append a trailing slash, as you can see in my reproducible example here:

https://github.com/hlolli/gatsby-bug-reproduction

Steps to reproduce

Clone https://github.com/hlolli/gatsby-bug-reproduction

build with --prefix-paths and start a server via the expressjs script named server.js (also under yarn serve).

Open in browser: http://localhost:3000/de

Expected result

http://localhost:3000/de stays

Actual result

http://localhost:3000/de

becomes

http://localhost:3000/de/

Hint: disable javascript and open http://localhost:3000/de again, and you'll see that the trailing slash will not get appended.

Environment

npx gatsby info --clipboard               

  System:
    OS: Linux 5.4 NixOS 20.03 (Markhor) 20.03.1577.74a80c5a9ab (Markhor)
    CPU: (8) x64 Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
    Shell: 3.0.2 - /run/current-system/sw/bin/fish
  Binaries:
    Node: 13.8.0 - /run/current-system/sw/bin/node
    Yarn: 1.22.0 - /run/current-system/sw/bin/yarn
    npm: 6.13.6 - /run/current-system/sw/bin/npm
  Languages:
    Python: 3.8.1 - /run/current-system/sw/bin/python
  Browsers:
    Firefox: 75.0
  npmPackages:
    gatsby: ^2.21.13 => 2.21.13
  npmGlobalPackages:
    gatsby-cli: 2.8.13

p.s. https://www.gatsbyjs.org/packages/gatsby-plugin-remove-trailing-slashes/ was of no help. As well as adding redirect via <Navigate from="/de/" to="/de" /> this will cause gatsby to throw bunch of errors as far as I remember.

4lm commented 4 years ago

Maybe I found the reason for the trailing slash issue in the prefixed base path!

If I create a gatsby-node.js life-cycle file in the root folder of the demo project gatsby-bug-reproduction and console log the page creation with exports.onCreatePage = props => console.log(props) I can see, that:

The code of "gatsby-plugin-remove-trailing-slashes" doesn't take basepath or pathPrefix into account and it doesn't remove the slash if the value of path in page is "/", compare with source code in gatsby-plugin-remove-trailing-slashes.

IMO this could be fixed by taking basepath and path in page in "gatsby-plugin-remove-trailing-slashes" into account (this way, people who want the trailing slash in a prefixed base path, can keep it, and people who don't, can use the plugin).

What do you think?

PS: If my suggestion is the way to go, this documentation might also be adjusted.

hlolli commented 4 years ago

@4lm thank so much for looking into this problem. You are correct in your analysis. The comfort of prefix paths in gatsby is that they are added afterwards, meaning one can always change the path endpoints without changing any code. But that also means that the root page is stored as "/" and removal of it would mean an empty string, which is currently not legal (but that could be changed?). And this makes fixing this problematic because using the build-time hooks, will not help even for a workaround, the mechanisms for the pagePath are way deeper than prefixPaths.

4lm commented 4 years ago

@hlolli yes, I see it now, you are right, trying to solve this via build-time API hooks doesn't work!

So, I looked a little deeper and found out, that in "production" the contents of production-app.js are called. In the code, I can see, that:

If I change the concerning code in production-app.js, from:

    navigate(__BASE_PATH__ + pagePath + browserLoc.search + browserLoc.hash, {
      replace: true,
    })

to:

    const to = (__BASE_PATH__ + pagePath + browserLoc.search + browserLoc.hash).replace(/\/+$/, "")
    navigate(to {
      replace: true,
    })

[the regular expression in .replace() removes all trailings slashes]

The result is your expected behavior of:

http://localhost:3000/de

IMO changing this code in a PR could be rejected, because it changes an introduced behavior for everyone. Maybe solving this via a plugin could be a solution, but how?

What do you think?

4lm commented 4 years ago

Mhhh 🤔, I get some error messages in the console:

VM63:1 GET http://localhost:3000/de/page-data/de/page-data.json 404 (Not Found)
VM63:1 GET http://localhost:3000/de/page-data/404.html/page-data.json 404 (Not Found)
production-app.js:128 Uncaught (in promise) Error: page resources for /de not found. Not rendering React
    at production-app.js:128

So more code has to be refactored, a simple regular expression seems to not be enough ...

4lm commented 4 years ago

After investigating the errors, I come to the conclusion, that at least the direction I took could be still feasible, but also, that a simple regex added as code change is not enough:

VM63:1 GET http://localhost:3000/de/page-data/de/page-data.json 404 (Not Found)
VM63:1 GET http://localhost:3000/de/page-data/404.html/page-data.json 404 (Not Found)
production-app.js:128 Uncaught (in promise) Error: page resources for /de not found. Not rendering React
    at production-app.js:128

IMO maybe fixable, but looks like a bigger operation now and the question remains, if a PR could be rejected, because it changes an introduced behavior for everyone ...

pzmudzinski commented 4 years ago

@4lm have you found any workaround for this?

4lm commented 4 years ago

@pzmudzinski above description is a direction, a workaround could go (and the only direction I can think of, to be honest). IMO the root of the problem is deeply nested and as described, needs code changes in multiple places in Gatsby itself (I see no simple plugin or API-hook fix).

I think, if a PR wants to have a chance to be accepted (for Gatsby V2), it should:

Personally, I don't work on this issue right now. So, if someone else is faster in solving this - you're welcome!

PS: IMO for a Gatsby V3 release, the default behavior could be then swapped with the optional behavior (no trailing slashes in prefixed root paths would be the default then).

PPS: Either way, both, trailing and no trailing slashes on prefixed root paths should IMO always be possible, because a website with an introduced behavior could lose juice in the SEO game, according to this article, if it changes introduced behavior concerning the trailing slashes of prefixed root paths.

github-actions[bot] commented 4 years ago

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here. If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open! As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

github-actions[bot] commented 4 years ago

Hey again!

It’s been 30 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it. Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to reopen this issue or create a new one if you need anything else. As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks again for being part of the Gatsby community! 💪💜

janpio commented 1 year ago

I stumbled over the same problem, and think I found a) a (very ugly) workaround how to modify your application to not have that trailing slash (redirect) for the root page and b) can imagine that this workaround could be turned into an actual feature in Gatsby that would make this behavior configurable.

Here is my discussion here where I documented my journey: https://github.com/gatsbyjs/gatsby/discussions/36971