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.

Sign up for GitHub

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

Handle Errors Gracefully #78

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Handle Errors Gracefully #78

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c4ef23
#73 stat files before serveIndex, also output date, size, and type
Aug 13, 2018
bbfd384
use dir as string, not object
Aug 13, 2018
af9d2be
#73 update tests for first PR
Aug 13, 2018
cc2c211
#76 Handle fs.stat errors gracefully
Aug 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,11 @@ function serveIndex(root, options) {
fstat(path, files, function (err, stats) {
if (err) return next(err);

// combine the stats into the file list
// combine the stats into the file list,
// ignoring ENOENT / null stat objects
var fileList = files.map(function (file, i) {
return { name: file, stat: stats[i] };
});
}).filter(function (file) { return file.stat });
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this actually be filtering anything out since the code below is providing a stat object even on error?


// sort file list
fileList.sort(fileSort);
Expand Down Expand Up @@ -570,7 +571,16 @@ function fstat(dir, files, cb) {
files.forEach(function(file){
batch.push(function(done){
fs.stat(join(dir, file), function(err, stat){
if (err && err.code !== 'ENOENT') return done(err);
// communicate errors via fake stat
if (err && err.code !== 'ENOENT') {
stat = {
size: 0,
mtime: new Date(0),
error: err.toString(),
code: err.code,
isDirectory: function () { return false }
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe your changes will expose this stat object to the users in the previous commits. I assume you made these fake values to prevent code from breaking, but it'll still break since you didn't provide all the values in a native Node.js stat object here. You'll want to make sure all the properties are covered if you're going to provide them at all, otherwise that's going to be even more confusing to the user, imo.

Copy link
Author

Choose a reason for hiding this comment

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

I just pushed another commit to the stat-first branch that converts to a w3 FileAPI-esque object sooner, which corrects that issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will that show up in this pr at all?

Copy link
Author

Choose a reason for hiding this comment

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

No, I made separate branches for each change and only based each change on the fewest number of commits needed to demonstrate the change.

All of the PRs are in order. I’ll go back and add when other PR the depend on.

}
}

// pass ENOENT as null stat, not error
done(null, stat || null);
Expand Down