-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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.
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
fix(gatsby): trailing slash redirect in commands/serve #21203
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -14,6 +14,7 @@ const telemetry = require(`gatsby-telemetry`) | |||||||||||||||||||||||||||||||||||||||||||
const detectPortInUseAndPrompt = require(`../utils/detect-port-in-use-and-prompt`) | ||||||||||||||||||||||||||||||||||||||||||||
const getConfigFile = require(`../bootstrap/get-config-file`) | ||||||||||||||||||||||||||||||||||||||||||||
const preferDefault = require(`../bootstrap/prefer-default`) | ||||||||||||||||||||||||||||||||||||||||||||
const fileExtensionRegEx = /.+\..+$/g | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
onExit(() => { | ||||||||||||||||||||||||||||||||||||||||||||
telemetry.trackCli(`SERVE_STOP`) | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -40,6 +41,29 @@ const readMatchPaths = async program => { | |||||||||||||||||||||||||||||||||||||||||||
return JSON.parse(rawJSON) | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||
* Adds trailing slash to request.url, in order to deceive static middleware | ||||||||||||||||||||||||||||||||||||||||||||
* into getting a directory request, thus preventing redirect. | ||||||||||||||||||||||||||||||||||||||||||||
* This is an express middleware to be used before static middleware | ||||||||||||||||||||||||||||||||||||||||||||
* @param {*} req | ||||||||||||||||||||||||||||||||||||||||||||
* @param {*} res | ||||||||||||||||||||||||||||||||||||||||||||
* @param {*} next | ||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||
const directoryRedirectPrevention = (req, res, next) => { | ||||||||||||||||||||||||||||||||||||||||||||
// url is home page | ||||||||||||||||||||||||||||||||||||||||||||
if (req.url === `/`) return next() | ||||||||||||||||||||||||||||||||||||||||||||
// url has trailing slash, no need to intervene | ||||||||||||||||||||||||||||||||||||||||||||
if (req.url.slice(-1) === `/`) return next() | ||||||||||||||||||||||||||||||||||||||||||||
// url is a file request, no need for trailing slash | ||||||||||||||||||||||||||||||||||||||||||||
if (fileExtensionRegEx.test(req.url)) return next() | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to overshoot a little bit. This is basically the Also, I think the |
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
// turning /slug into /slug/ | ||||||||||||||||||||||||||||||||||||||||||||
req.originalUrl += `/` // https://expressjs.com/en/api.html#req.originalUrl | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first example in this link has:
So potentially this line would be doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, url's with query args is a use case I haven't tackled yet. I'm not fully familiar with gatsby's handle of query args. Your input here is required. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I have no idea :) I mainly jumped on the syntactical issues with this part of the change set. Maybe @pieh knows There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have this piece of code in production runtime that adjust url to use canonical page path for initial page load (so this will be connected with to gatsby/packages/gatsby/cache-dir/production-app.js Lines 99 to 119 in 9d3a8f0
Just note that "canonical path" on its own doesn't specify if it has trailing slash or not - it just mean I'm not sure if this make any sense or if this is answer any question here. But for manipulating URLs I recommend using builtin Node's
|
||||||||||||||||||||||||||||||||||||||||||||
req.url += `/` | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which made me look into
So it looks like |
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
return next() | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const matchPathRouter = (matchPaths, options) => (req, res, next) => { | ||||||||||||||||||||||||||||||||||||||||||||
const { url } = req | ||||||||||||||||||||||||||||||||||||||||||||
if (req.accepts(`html`)) { | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -85,6 +109,7 @@ module.exports = async program => { | |||||||||||||||||||||||||||||||||||||||||||
app.use(telemetry.expressMiddleware(`SERVE`)) | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
router.use(compression()) | ||||||||||||||||||||||||||||||||||||||||||||
router.use(directoryRedirectPrevention) | ||||||||||||||||||||||||||||||||||||||||||||
router.use(express.static(`public`)) | ||||||||||||||||||||||||||||||||||||||||||||
const matchPaths = await readMatchPaths(program) | ||||||||||||||||||||||||||||||||||||||||||||
router.use(matchPathRouter(matchPaths, { root })) | ||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second check subsumes the first because
'x'.slice(-1) === 'x'
:)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, I wanted to differentiate the two cases, easier to understand.
Falls into "write code for humans" practice. The fact that you noticed, proves my point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get what you're saying but I still think I'd prefer to drop the redundant check and updating the comment to reflect the two cases being caught here.