Fix 301 redirect to trailing slash when not specified in createPage.path #19567

adrienharnay · 2019-11-16T23:52:05Z

Description

When using gatsby serve, for pages created with createPage, for a given path /hello there is a 301 redirect to /hello/ even when path does not end with a slash.

The issue can be reproduced on this repository.

The fix is to keep generating public/hello/index.html if createPage.path === '/hello/', but instead to generate public/hello.html if createPage.path === '/hello.

Related Issues

Fixes #19543

adrienharnay · 2019-11-17T14:23:49Z

Tests pass locally, maybe I missed something?

pieh · 2019-11-18T15:54:17Z

packages/gatsby/src/utils/worker/render-html.js

@@ -7,7 +7,8 @@ const generatePathToOutput = outputPath => {
  let outputFileName = outputPath.replace(/^(\/|\\)/, ``) // Remove leading slashes for webpack-dev-server

  if (!/\.(html?)$/i.test(outputFileName)) {
-    outputFileName = path.join(outputFileName, `index.html`)
+    outputFileName =
+      outputFileName + `${outputFileName.endsWith(`/`) ? `index` : ``}.html`


I think this is dangerous - it relies on hosting matching /path/something to /path/something.html which I'm not sure how common is (probably less common than matching index.html).

Do you have any ideas on how we could test this? Are you just thinking this is a difference in OS's potentially?

Ah, I see your point. Do you have any other idea to avoid the 301 redirect? For example, loading this page triggers it: https://reactjs.org/languages

Do you have any ideas on how we could test this?

Other than trying it out on top X of hosting services/software (or reading docs for them), I see no other way :(

Ah, I see your point. Do you have any other idea to avoid the 301 redirect?

Ideally we would only change gatsby serve here and not the output that Gatsby produces right now (that change have breaking potential). Ideally we could configure express.static to not do redirects, but last time I checked this wasn't possible. We might need to:

make feature request to serve-static to make 301 behaviour configurable (so it use regular 200 response instead of 301 for those paths)

vendor serve-static and apply patch we need ourselves?

or look for something that will not do 301 instead of serve-static?

(serve-static is package used by express.static)

also, fact that we need to change express.static config to support new format is what I worry about with this change

I understand the implications... And I see why my changes are too dangerous.

What about... looping through the public folder instead of using express.static()? Pseudo-code:

const serve = (dir = './public') => { const folders = listFolders(dir); const files = listFiles(dir); for each folder in folders { serve(dir + folder); } [custom logic to serve HTML files without trailing slash] }

Few additional alternatives:

We can add "200 redirect" ourselves in express middleware before we register express.static:

router.use((req, res, next) => { if (!/.+\..+$/g.test(req.url) && req.url.slice(-1) !== '/') { req.url = `${req.url}/`; } next(); })

this would "200 redirect non-slash non-extension requests"

Problem is that those might be actual files :/

We can use redirect: false (and maybe fallthrough: true) in option for express.static and add new route handler after express.static to try to send file on our own?

router.use((req, res, next) => { if (!/.+\..+$/g.test(req.url) && req.url.slice(-1) !== '/') { // let's try adding trailing `/index.html`, see if file exist and send it if it does } else { next(); } })

This is a very good idea. I just updated the PR, and also updated the test repository, it works well! Just re-install node_modules and run patch-package to test it.

blainekasten · 2019-11-21T15:08:32Z

packages/gatsby/src/commands/serve.js

  const matchPaths = await readMatchPaths(program)
  router.use(matchPathRouter(matchPaths, { root }))
+  router.use((req, res, next) => {
+    if (!/.+\..+$/g.test(req.url) && req.url.slice(-1) !== `/`) {


for clarity, could we store this regex in a variable that has a descriptive name? mentally grokking this regex is a bit confounding 😆

or at least add a comment above explaining what this is doing, what it's solving, etc. I'm just worried about the long term maintenance of this fix getting lost in 6-12m

this is copied from my examples, and this is probably not good test to have in retrospect (it infer that resource should be file if it has . in it, but that's true at all), so we should remove at least first part of condition leaving only test for trailing slash.

I wonder what we will get now for legitimate 404s tho, now that serve-static won't immediately return 404

blainekasten · 2019-12-02T14:08:44Z

@adrienharnay any chance for an update?

adrienharnay · 2019-12-03T08:44:15Z

Hey, sorry for being silent so long! I've updated the PR (and the example repository), and tested that the 404s behave the same before and after the patch (they do). Please let me know if I can do anything else :)
cc. @blainekasten @pieh

blainekasten

This looks great. I'll get this merged and released soon! Thanks for your hard work and going back and forth with us @adrienharnay !

blainekasten · 2019-12-03T19:49:23Z

@adrienharnay unfortunately, it looks like this fix is breaking some other things. Our e2e tests for image path prefixes is breaking. Here's a screenshot from the issue.

I suppose we probably should check that the file exists before sending it?

adrienharnay · 2019-12-07T14:36:03Z

Maybe the tests are testing an implementation detail of the previous 404 page, and now it's changed so the test fails? Can you give me pointers on what is exactly tested here please, not sure I understand the error message

wardpeet · 2019-12-09T14:34:14Z

I'm not 100% sure this is the correct path for gatsby serve. When running gatsby build and you upload to a production server, by default, it will need /hi/ as a URL. It looks like gatsby develop isn't mimicking gatsby serve's/production behavior. So, I would suggest adding a 301 to develop instead.

I think this approach also fixes your issue.

The above comment is just my opinion, so I'm happy to hear your views.

fabiosantoscode · 2020-02-04T20:33:32Z

packages/gatsby/src/commands/serve.js

+    if (req.url.slice(-1) !== `/`) {
+      res.sendFile(req.url + `/index.html`, {
+        root,
+      })


What if the file doesn't exist? A callback argument can be added here to deal with this case and allow for the regular 404 logic to occur:

res.sendFile(...., (error) => { if (error) next() // if there's no error, request.end() has been called and there's nothing to do. })

pvdz · 2020-04-20T09:53:34Z

@adrienharnay @wardpeet the diff in this PR is rather small. Can we come to a conclusion on what to do here? Merge or close the PR?

adrienharnay · 2020-04-20T14:32:03Z

Well, I'm still convinced the choice to force trailing slashes or not should belong to the user, and this PR solves this. But I won't lie, the lag on this PR got me pretty demotivated, and I now depend on a fork. If the maintainers want me to finalize the PR so that it can get merged, I'll gladly do it though.

wardpeet · 2020-04-27T16:58:29Z

I've been playing with this PR a bit and what a "default" webserver does. I'm wondering why does gatsby serve needs to mimic the same solution as a host like netlify? Is it e2e tests?

My demo of a default nginx/apache server running on Plesk gives me the same behaviour as serve gives me.
https://gatsby-blog-no-slash.wardpeet.dev/new-beginnings

fabiosantoscode · 2020-04-27T19:35:59Z

@wardpeet I had a look at what's happening at the HTTP level. Gatsby and the server disagree on whether a trailing slash is needed.

fabio@fabio-thinkpad ♥  curl -I https://gatsby-blog-no-slash.wardpeet.dev/new-beginnings/
HTTP/1.1 200 OK
[...]

fabio@fabio-thinkpad ♥  curl -I https://gatsby-blog-no-slash.wardpeet.dev/new-beginnings
HTTP/1.1 301 Moved Permanently
[...]
Location: https://gatsby-blog-no-slash.wardpeet.dev/new-beginnings/

So really gatsby is hiding the redirects the server is doing by doing its own clientside redirects. Our team had a similar issue with gatsby before, and the google SEO tools told us about it.

Eventually we moved to serving our things from AWS S3 so we stopped needing this, but I'm very partial to slash-less URLs. They're just pretty :)

wardpeet · 2020-04-27T20:18:27Z

So really gatsby is hiding the redirects the server is doing by doing its own clientside redirects. Our team had a similar issue with gatsby before, and the google SEO tools told us about it.

I'm aware of gatsby is hiding this implementation and that a redirection is happening. The problem is that when we build, we always build like you have trailing slashes and it's the server implementation that needs to handle pretty URLs, not us.

gatsby serve is not a production server, it's merely there to run e2e-tests, do a final check if everything is looking alright.

So I'm still unsure what the use case is of not doing a 301 in serve.

wardpeet · 2020-05-12T07:23:52Z

Hey @adrienharnay! First and foremost, I want to thank you for opening this PR and taking the time to improve Gatsby! It has been 14 days until a response. Gatsby serve is doing exactly what a basic webserver would do. Serve is a small shell around express. You can create your own if you need to.

avinson · 2020-05-12T19:43:47Z

Eventually we moved to serving our things from AWS S3 so we stopped needing this, but I'm very partial to slash-less URLs. They're just pretty :)

@fabiosantoscode If I can ask, how did you fix this in S3? I still see this redirect behavior using standard gatsby build and s3 plugin.

adrienharnay added 3 commits November 16, 2019 16:41

Update serve.js

47cc1a6

fix done

9a286db

Update render-html.js

f83a3e9

adrienharnay requested a review from a team as a code owner November 16, 2019 23:52

blainekasten self-assigned this Nov 18, 2019

pieh reviewed Nov 18, 2019

View reviewed changes

fix

4ee3eca

blainekasten reviewed Nov 21, 2019

View reviewed changes

Update serve.js

d4f4668

chore: format

d5a7e31

blainekasten approved these changes Dec 3, 2019

View reviewed changes

fabiosantoscode mentioned this pull request Feb 4, 2020

Add option to remove trailing slashes, as opposed to adding them expressjs/serve-static#132

Closed

fabiosantoscode reviewed Feb 4, 2020

View reviewed changes

cosmn mentioned this pull request Feb 4, 2020

fix(gatsby): trailing slash redirect in commands/serve #21203

Closed

wardpeet self-assigned this Mar 2, 2020

wardpeet closed this May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 301 redirect to trailing slash when not specified in createPage.path #19567

Fix 301 redirect to trailing slash when not specified in createPage.path #19567

adrienharnay commented Nov 16, 2019 •

edited

adrienharnay commented Nov 17, 2019

pieh Nov 18, 2019 •

edited

blainekasten Nov 18, 2019

adrienharnay Nov 18, 2019

pieh Nov 18, 2019

pieh Nov 18, 2019

adrienharnay Nov 18, 2019

pieh Nov 18, 2019

adrienharnay Nov 19, 2019

blainekasten Nov 21, 2019

blainekasten Nov 21, 2019

pieh Nov 21, 2019

blainekasten commented Dec 2, 2019

adrienharnay commented Dec 3, 2019

blainekasten left a comment

blainekasten commented Dec 3, 2019 •

edited

adrienharnay commented Dec 7, 2019 •

edited

wardpeet commented Dec 9, 2019

fabiosantoscode Feb 4, 2020

pvdz commented Apr 20, 2020

adrienharnay commented Apr 20, 2020

wardpeet commented Apr 27, 2020

fabiosantoscode commented Apr 27, 2020 •

edited

wardpeet commented Apr 27, 2020 •

edited

wardpeet commented May 12, 2020

avinson commented May 12, 2020

Fix 301 redirect to trailing slash when not specified in createPage.path #19567

Fix 301 redirect to trailing slash when not specified in createPage.path #19567

Conversation

adrienharnay commented Nov 16, 2019 • edited

Description

Related Issues

adrienharnay commented Nov 17, 2019

pieh Nov 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blainekasten commented Dec 2, 2019

adrienharnay commented Dec 3, 2019

blainekasten left a comment

Choose a reason for hiding this comment

blainekasten commented Dec 3, 2019 • edited

adrienharnay commented Dec 7, 2019 • edited

wardpeet commented Dec 9, 2019

Choose a reason for hiding this comment

pvdz commented Apr 20, 2020

adrienharnay commented Apr 20, 2020

wardpeet commented Apr 27, 2020

fabiosantoscode commented Apr 27, 2020 • edited

wardpeet commented Apr 27, 2020 • edited

wardpeet commented May 12, 2020

avinson commented May 12, 2020

adrienharnay commented Nov 16, 2019 •

edited

pieh Nov 18, 2019 •

edited

blainekasten commented Dec 3, 2019 •

edited

adrienharnay commented Dec 7, 2019 •

edited

fabiosantoscode commented Apr 27, 2020 •

edited

wardpeet commented Apr 27, 2020 •

edited