-
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?
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 |
---|---|---|
|
@@ -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 | ||
middleware({ | ||
url: req.url, | ||
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 commentThe 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 | ||
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. 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), | ||
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 comma looks extraneous. |
||
}, res, next); | ||
} | ||
} | ||
else if (err) { | ||
status[500](res, next, { error: err }); | ||
|
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')); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
200.html fallback!!! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
index!!! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
pageTwo!!! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
subdir index!!! |
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.