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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 16 additions & 3 deletions lib/ecstatic.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,26 @@ var ecstatic = module.exports = function (dir, options) {
url: parsed.pathname + '.' + defaultExt + ((parsed.search)? parsed.search:'')
}, res, next);
}
else {
// Try to serve default ./404.html
else if (!handleError) {
// If we're not handling errors/fallbacks, bail out now
Copy link
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.

middleware({
url: req.url,
statusCode: 404
}, res, next);
}
else if (path.basename(parsed.pathname, defaultExt) === '200.') {
Copy link
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.

// Already tried ./200.html, now try ./404.html
middleware({
url: (handleError ? ('/' + path.join(baseDir, '404.' + defaultExt)) : req.url),
url: '/' + path.join(baseDir, '404.' + defaultExt),
statusCode: 404
}, res, next);
}
else {
// Try ./200.html before falling back to ./404.html
Copy link
Owner

@jfhbrook jfhbrook Sep 22, 2016

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.

middleware({
url: '/' + path.join(baseDir, '200.' + defaultExt),
Copy link
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.

}, res, next);
}
}
else if (err) {
status[500](res, next, { error: err });
Expand Down
29 changes: 29 additions & 0 deletions test/fallback-200.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
var test = require('tap').test,
ecstatic = require('../'),
http = require('http'),
request = require('request'),
eol = require('eol');

function runTest(path, expectedBody) {
return function (t) {
t.plan(3);
var server = http.createServer(ecstatic(__dirname + '/public/containsFallback'));

server.listen(0, function () {
var port = server.address().port;
request.get('http://localhost:' + port + path, function (err, res, body) {
t.ifError(err);
t.equal(res.statusCode, 200);
t.equal(eol.lf(body), expectedBody);
server.close(function() { t.end(); });
});
});
};
}

test('fallback showsIndex', runTest('/', 'index!!!\n'));
test('fallback showsNonIndex', runTest('/pageTwo.html', 'pageTwo!!!\n'));
test('fallback showsSubDir', runTest('/subdir', 'subdir index!!!\n'));
test('fallback fallsBack 1', runTest('/something', '200.html fallback!!!\n'));
test('fallback fallsBack 2', runTest('/else.html', '200.html fallback!!!\n'));
test('fallback fallsBack 3', runTest('/with/multiple/paths', '200.html fallback!!!\n'));
1 change: 1 addition & 0 deletions test/public/containsFallback/200.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
200.html fallback!!!
1 change: 1 addition & 0 deletions test/public/containsFallback/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
index!!!
1 change: 1 addition & 0 deletions test/public/containsFallback/pageTwo.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pageTwo!!!
1 change: 1 addition & 0 deletions test/public/containsFallback/subdir/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
subdir index!!!