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

stat files before serveIndex, also output date, size, and type #74

Closed
wants to merge 7 commits into from
Closed

stat files before serveIndex, also output date, size, and type #74

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 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
79d5572
convert from fs.stat to vanilla json before response templates
Aug 13, 2018
cb76221
add a single trailing slash to directory name
Aug 13, 2018
e9b98d2
use bytes package instead of inline sizer
Aug 13, 2018
23ba584
check Accept header after directory status
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
137 changes: 94 additions & 43 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,13 @@ function serveIndex(root, options) {
return;
}

// content-negotiation
var accept = accepts(req);
var type = accept.type(mediaTypes);

// not acceptable
if (!type) return next(createError(406));
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to break using this module above your other handlers. This will block all request that doesn't have the right Accept header even if this module is just going to do nothing because the folder doesn't exist.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense.

I originally moved it up so that it would be checked before all of the fs.stat()s. I moved it just after the directory stat and before the fs.readdir.


// parse URLs
var url = parseUrl(req);
var originalUrl = parseUrl.original(req);
Expand Down Expand Up @@ -154,13 +161,26 @@ function serveIndex(root, options) {
});
files.sort();

// content-negotiation
var accept = accepts(req);
var type = accept.type(mediaTypes);
// add parent directory as first
if (showUp) {
files.unshift('..');
}

// stat all files
fstat(path, files, function (err, stats) {
if (err) return next(err);

// not acceptable
if (!type) return next(createError(406));
serveIndex[mediaType[type]](req, res, files, next, originalDir, showUp, icons, path, view, template, stylesheet);
// combine the stats into the file list
var fileList = files.map(function (file, i) {
return { name: file, stat: stats[i] };
});

// sort file list
fileList.sort(fileSort);

var dir = { name: originalDir, stat: stat };
serveIndex[mediaType[type]](req, res, dir, fileList, next, showUp, icons, path, view, template, stylesheet);
});
});
});
};
Expand All @@ -170,46 +190,29 @@ function serveIndex(root, options) {
* Respond with text/html.
*/

serveIndex.html = function _html(req, res, files, next, dir, showUp, icons, path, view, template, stylesheet) {
serveIndex.html = function _html(req, res, dir, files, next, showUp, icons, path, view, template, stylesheet) {
var render = typeof template !== 'function'
? createHtmlRender(template)
: template

if (showUp) {
files.unshift('..');
}

// stat all files
stat(path, files, function (err, stats) {
// read stylesheet
fs.readFile(stylesheet, 'utf8', function (err, style) {
if (err) return next(err);

// combine the stats into the file list
var fileList = files.map(function (file, i) {
return { name: file, stat: stats[i] };
});

// sort file list
fileList.sort(fileSort);
// create locals for rendering
var locals = {
directory: dir.name,
displayIcons: Boolean(icons),
fileList: files,
path: path,
style: style,
viewName: view
};

// read stylesheet
fs.readFile(stylesheet, 'utf8', function (err, style) {
// render html
render(locals, function (err, body) {
if (err) return next(err);

// create locals for rendering
var locals = {
directory: dir,
displayIcons: Boolean(icons),
fileList: fileList,
path: path,
style: style,
viewName: view
};

// render html
render(locals, function (err, body) {
if (err) return next(err);
send(res, 'text/html', body)
});
send(res, 'text/html', body)
});
});
};
Expand All @@ -218,16 +221,49 @@ serveIndex.html = function _html(req, res, files, next, dir, showUp, icons, path
* Respond with application/json.
*/

serveIndex.json = function _json(req, res, files) {
send(res, 'application/json', JSON.stringify(files))
serveIndex.json = function _json(req, res, dir, files) {
var directory = {
name: dir.name,
type: 'inode/directory',
size: dir.stat.size,
lastModified: dir.stat.mtime.toISOString()
}

// similar to W3 FileAPI Object
var nodes = files.map(function (file) {
var ext = extname(file.name)
var mimetype = mime.lookup(ext)
return {
name: file.name,
type: file.stat.isDirectory() ? 'inode/directory' : mimetype,
size: file.stat.size,
lastModified: file.stat.mtime.toISOString()
}
})
send(res, 'application/json', JSON.stringify({ directory: directory, nodes: nodes }))
};

/**
* Respond with text/plain.
*/

serveIndex.plain = function _plain(req, res, files) {
send(res, 'text/plain', (files.join('\n') + '\n'))
serveIndex.plain = function _plain(req, res, dir, files) {
var directory = {
name: dir.name.replace(/\/$/, '\/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this replace actually do anything? I can't figure out what it does, can you ellaborate?

Copy link
Author

Choose a reason for hiding this comment

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

change to dir.name.replace(/\/?$/, '/')
/ => /
demo => demo/

};

// include size and date
var nodes = files.map(function (file) {
// human readable
var size = hsize(file.stat.size)

return [
file.stat.mtime.toISOString(),
size,
file.name + (file.stat.isDirectory() ? '/' : '')
].join('\t')
})
send(res, 'text/plain', (directory.name + '\n' + nodes.join('\n') + '\n'))
};

/**
Expand Down Expand Up @@ -506,12 +542,27 @@ function send (res, type, body) {
res.end(body, 'utf8')
}

/**
* Convert size to human-readable size
*/

var SIZES = [ 'B', 'K', 'M', 'G', 'T', 'P', 'E' ]
function hsize(size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a module here instead of adding this code to the module. The bytes module is one I know, but you're welcome to use a different one if there is one you like better.

Copy link
Author

Choose a reason for hiding this comment

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

That makes me sad. There's so many packages in node that do next to nothing and somehow end up including 100's of megabytes of download. The bytes package is WAY overkill for something this simple.

That said, I'll make the change.

Copy link
Author

Choose a reason for hiding this comment

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

(especially considering the recent fiasco with eslint - who knows what percentage of npm was compromised from that alone)

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be a diff package if there is a better one like I said. The other alternative is that this would need tests to cover the conversations (for instance I deleted the "E" from the list and no tests seemed to catch that -- I also changed 1024 to 999 and nothing failed) and it creates more things I have to maintain in the long term, which is harder when it doesn't have tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

(especially considering the recent fiasco with eslint - who knows what percentage of npm was compromised from that alone)

Ok, but why would it be any different that this module itself cannot be compromised?

var i = 0
while (size > 1024 && i < SIZES.length) {
size = Math.floor(size/1024)
i += 1
}
return size + SIZES[i]
}


/**
* Stat all files and return array of stat
* in same order.
*/

function stat(dir, files, cb) {
function fstat(dir, files, cb) {
var batch = new Batch();

batch.concurrency(10);
Expand Down
14 changes: 7 additions & 7 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,9 +504,9 @@ describe('serveIndex(root)', function () {
it('should get file list', function (done) {
var server = createServer()

serveIndex.html = function (req, res, files) {
serveIndex.html = function (req, res, dir, files) {
var text = files
.filter(function (f) { return /\.txt$/.test(f) })
.filter(function (f) { return /\.txt$/.test(f.name) })
.sort()
res.setHeader('Content-Type', 'text/html')
res.end('<b>' + text.length + ' text files</b>')
Expand All @@ -521,9 +521,9 @@ describe('serveIndex(root)', function () {
it('should get dir name', function (done) {
var server = createServer()

serveIndex.html = function (req, res, files, next, dir) {
serveIndex.html = function (req, res, dir, files, next) {
res.setHeader('Content-Type', 'text/html')
res.end('<b>' + dir + '</b>')
res.end('<b>' + dir.name + '</b>')
}

request(server)
Expand All @@ -535,7 +535,7 @@ describe('serveIndex(root)', function () {
it('should get template path', function (done) {
var server = createServer()

serveIndex.html = function (req, res, files, next, dir, showUp, icons, path, view, template) {
serveIndex.html = function (req, res, dir, files, next, showUp, icons, path, view, template) {
res.setHeader('Content-Type', 'text/html')
res.end(String(fs.existsSync(template)))
}
Expand All @@ -549,7 +549,7 @@ describe('serveIndex(root)', function () {
it('should get template with tokens', function (done) {
var server = createServer()

serveIndex.html = function (req, res, files, next, dir, showUp, icons, path, view, template) {
serveIndex.html = function (req, res, dir, files, next, showUp, icons, path, view, template) {
res.setHeader('Content-Type', 'text/html')
res.end(fs.readFileSync(template, 'utf8'))
}
Expand All @@ -567,7 +567,7 @@ describe('serveIndex(root)', function () {
it('should get stylesheet path', function (done) {
var server = createServer()

serveIndex.html = function (req, res, files, next, dir, showUp, icons, path, view, template, stylesheet) {
serveIndex.html = function (req, res, dir, files, next, showUp, icons, path, view, template, stylesheet) {
res.setHeader('Content-Type', 'text/html')
res.end(String(fs.existsSync(stylesheet)))
}
Expand Down