-
Notifications
You must be signed in to change notification settings - Fork 194
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
Use 200.html as a fallback before 404.html #190
base: master
Are you sure you want to change the base?
Conversation
Hey, just now seeing this. Will try to review in the next few days. |
else { | ||
// Try ./200.html before falling back to ./404.html | ||
middleware({ | ||
url: '/' + path.join(baseDir, '200.' + defaultExt), |
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.
This comma looks extraneous.
statusCode: 404 | ||
}, res, next); | ||
} | ||
else if (path.basename(parsed.pathname, defaultExt) === '200.') { |
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.
This check seems brittle and awkward, and I don't think anyone else does it. Do you see a way around it? I don't, least not without a bigger refactor of ecstatic around passing state (which I want to do, but is kind of a whole thing).
I'll have to think about this.
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.
This would have never occurred to me.
statusCode: 404 | ||
}, res, next); | ||
} | ||
else { | ||
// Try ./200.html before falling back to ./404.html |
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.
It's mildly surprising that this isn't opt-in behavior, but 200.html wasn't used for anything else prior, right? So this is a breaking change, but that seems fine.
So, uh, yeah! Github's new review feature is okay. |
else { | ||
// Try to serve default ./404.html | ||
else if (!handleError) { | ||
// If we're not handling errors/fallbacks, bail out now |
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.
...is this a bug? I'll have to look more closely, but I think maybe handleError: false should have us be calling next directly.
Last thought: Is this testable? |
I lied: Do you think this PR solves a similar problem as #146 ? |
@jfhbrook it solves similar problem for me in a less flexible way. In my case, I'd love to be able to give custom fallback page, and not rely on convention 200/404.html pages |
I wouldn't have expected any special 200/404 file behaviour either, as the README doesn't mention it. |
Feel free to PR some clarifying docs @mk-pmb |
This is a convention I think makes a lot of sense for single page applications, when a URL doesn't match anything.
/200.html
and serve that with status404
/404.html
and serve that with status404
404
This was introduced by harp (I think) and is used by surge.sh and it seems to be really straightforward to use.
I've made it so that if you set the config of
handleError
tofalse
then this doesn't look for200.html
either, just to keep compatibility with other clients.