Skip to content

Use 200.html as a fallback before 404.html#190

Open
geelen wants to merge 3 commits into
jfhbrook:masterfrom
geelen:patch-1
Open

Use 200.html as a fallback before 404.html#190
geelen wants to merge 3 commits into
jfhbrook:masterfrom
geelen:patch-1

Conversation

@geelen

@geelen geelen commented Sep 6, 2016

Copy link
Copy Markdown

This is a convention I think makes a lot of sense for single page applications, when a URL doesn't match anything.

  • First check for /200.html and serve that with status 404
  • Then check for /404.html and serve that with status 404
  • Then serve a blank 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 to false then this doesn't look for 200.html either, just to keep compatibility with other clients.

@jfhbrook

Copy link
Copy Markdown
Owner

Hey, just now seeing this. Will try to review in the next few days.

Comment thread lib/ecstatic.js
else {
// Try ./200.html before falling back to ./404.html
middleware({
url: '/' + path.join(baseDir, '200.' + defaultExt),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This comma looks extraneous.

Comment thread lib/ecstatic.js
statusCode: 404
}, res, next);
}
else if (path.basename(parsed.pathname, defaultExt) === '200.') {

Copy link
Copy Markdown
Owner

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.

@jfhbrook jfhbrook left a comment

Copy link
Copy Markdown
Owner

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.

Comment thread lib/ecstatic.js
}, res, next);
}
else {
// Try ./200.html before falling back to ./404.html

@jfhbrook jfhbrook Sep 22, 2016

Copy link
Copy Markdown
Owner

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.

@jfhbrook

Copy link
Copy Markdown
Owner

So, uh, yeah! Github's new review feature is okay.

Comment thread lib/ecstatic.js
else {
// Try to serve default ./404.html
else if (!handleError) {
// If we're not handling errors/fallbacks, bail out now

Copy link
Copy Markdown
Owner

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.

@jfhbrook

Copy link
Copy Markdown
Owner

Last thought: Is this testable?

@jfhbrook

Copy link
Copy Markdown
Owner

I lied:

Do you think this PR solves a similar problem as #146 ?

@slorber

slorber commented Nov 10, 2016

Copy link
Copy Markdown

@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

@mk-pmb

mk-pmb commented Oct 18, 2017

Copy link
Copy Markdown
Contributor

I wouldn't have expected any special 200/404 file behaviour either, as the README doesn't mention it.

@jfhbrook

Copy link
Copy Markdown
Owner

Feel free to PR some clarifying docs @mk-pmb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants