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

Changes from 4 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
11 changes: 10 additions & 1 deletion packages/gatsby/src/commands/serve.js
View file
Open in desktop
Expand Up @@ -85,9 +85,18 @@ module.exports = async program => {
app.use(telemetry.expressMiddleware(`SERVE`))

router.use(compression())
router.use(express.static(`public`))
router.use(express.static(`public`, { redirect: false }))
const matchPaths = await readMatchPaths(program)
router.use(matchPathRouter(matchPaths, { root }))
router.use((req, res, next) => {
if (!/.+\..+$/g.test(req.url) && req.url.slice(-1) !== `/`) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

res.sendFile(req.url + `/index.html`, {
root,
})
Copy link

Choose a reason for hiding this comment

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

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

} else {
next()
}
})
router.use((req, res, next) => {
if (req.accepts(`html`)) {
return res.status(404).sendFile(`404.html`, { root })
Expand Down