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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
47cc1a6
Update serve.js
adrienharnay Nov 16, 2019
9a286db
fix done
adrienharnay Nov 16, 2019
f83a3e9
Update render-html.js
adrienharnay Nov 16, 2019
4ee3eca
fix
adrienharnay Nov 19, 2019
d4f4668
Update serve.js
adrienharnay Dec 3, 2019
d5a7e31
chore: format
Dec 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/gatsby/src/commands/serve.js
View file
Open in desktop
Expand Up @@ -85,7 +85,7 @@ module.exports = async program => {
app.use(telemetry.expressMiddleware(`SERVE`))

router.use(compression())
router.use(express.static(`public`))
router.use(express.static(`public`, { extensions: [`index.html`] }))
const matchPaths = await readMatchPaths(program)
router.use(matchPathRouter(matchPaths, { root }))
router.use((req, res, next) => {
Expand Down
3 changes: 2 additions & 1 deletion packages/gatsby/src/utils/worker/render-html.js
View file
Open in desktop
Expand Up @@ -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`
Copy link
Contributor

@pieh pieh Nov 18, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. make feature request to serve-static to make 301 behaviour configurable (so it use regular 200 response instead of 301 for those paths)
  2. vendor serve-static and apply patch we need ourselves?
  3. or look for something that will not do 301 instead of serve-static?

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few additional alternatives:

  1. 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 :/

  1. 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();
    }
  })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return path.join(process.cwd(), `public`, outputFileName)
Expand Down