Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 9 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,14 @@ This is skipped if the requested file already has an extension.

##### index

By default send supports "index.html" files, to disable this
set `false` or to supply a new index pass a string or an array
in preferred order.
By default *send* supports "index.html" files. To disable this, set index option
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this section is about the index option, I don't think you need to refer to itself, as it ends up being really wordy.

to `false`, or you can be able to supply a new index passing a string with the
Copy link
Contributor

Choose a reason for hiding this comment

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

"you can be able to" doesn't sound right. I think probably just remove those words?

Copy link
Contributor

Choose a reason for hiding this comment

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

And passing would become just pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's formal english... As you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's formal english... As you want.

I'm sorry, I can't seem to figure out what your response means...

filename or an array in the preferred order.

Alternatively, you can pass a function that will generate the index. The
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than tacking this on as a separate paragraph, can you incorporate them all together? Otherwise, it sounds very hack in, but having a complete paragraph saying that the option only takes a boolean, string, or array, then another paragraph following to say "but wait! We forgot to add it takes a function too"

function will receive the filesystem absolute path and will have the
`SendStream` instance as `this` object. You should call internally to the
Copy link
Contributor

Choose a reason for hiding this comment

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

Having the "this" populated on a callback like function is very odd, and very easy to get lost. It is kind of annoying already for events, but typically I never see this on straight functions. Can everything a user would need just be passed as arguments?

`this.send()` method or try to mimic its behaviour.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the docs be clearer on how someone is supposed to "mimic its behavior"?


##### lastModified

Expand Down Expand Up @@ -229,7 +234,7 @@ var app = http.createServer(function onRequest (req, res) {
}).listen(3000)
```

## License
## License

[MIT](LICENSE)

Expand Down
29 changes: 23 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,20 @@ function SendStream (req, path, options) {
this._dotfiles = undefined
}

this._extensions = opts.extensions !== undefined
? normalizeList(opts.extensions, 'extensions option')
: []
this._extensions = normalizeList(opts.extensions, 'extensions option')

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a necessary change for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

normalizeList() now return an empty list when the input is undefined, so the check here is bloated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any changes to normalizeList in your PR. Perhaps I should be clearer: don't include changes in your PR that are not accomplishing your goal. These types of changes need to be different PRs. Please revert this change.

this._index = opts.index !== undefined
? normalizeList(opts.index, 'index option')
: ['index.html']
switch (typeof opts.index) {
case 'function':
this._index = opts.index
break

case 'undefined':
this._index = ['index.html']
break

default:
this._index = normalizeList(opts.index, 'index option')
}

this._lastModified = opts.lastModified !== undefined
? Boolean(opts.lastModified)
Expand Down Expand Up @@ -542,11 +549,16 @@ SendStream.prototype.pipe = function pipe (res) {
}

// index file support
if (typeof this._index === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this check just be moved into the sendIndex method? It is a public method, and it's weird the event emit is in there but not this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would in fact remove the index event at all... This would be moved there too, but then the check for the function would need to be added both on the next if conditional AND inside the sendIndex() function too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, I don't understand your response. Can you say it in a different way?

this._index(path)
return res
}
if (this._index.length && this.path[this.path.length - 1] === '/') {
this.sendIndex(path)
return res
}

// Render a file, or an index as a file
this.sendFile(path)
return res
}
Expand Down Expand Up @@ -703,6 +715,11 @@ SendStream.prototype.sendFile = function sendFile (path) {
* @api private
*/
SendStream.prototype.sendIndex = function sendIndex (path) {
if (listenerCount(this, 'index')) {
this.emit('index', path)
return
}

var i = -1
var self = this

Expand Down
39 changes: 38 additions & 1 deletion test/send.js
Original file line number Diff line number Diff line change
Expand Up @@ -702,9 +702,11 @@ describe('send(file).pipe(res)', function () {
.pipe(res)
})

var filePath = path.join(fixtures, 'pets', 'index.html')

request(app)
.get('/pets/')
.expect(200, fs.readFileSync(path.join(fixtures, 'pets', 'index.html'), 'utf8'), done)
.expect(200, fs.readFileSync(filePath, 'utf8'), done)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a necessary change for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ESlint was giving me errors of lines too long, this fixed it. Source code lines should never be longer than 80 columns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Revert the change if you would like this landed... Please don't include unrelated changes. If we wanted to enforce a max line length, we would do so, but we do not.

})
})

Expand Down Expand Up @@ -1115,6 +1117,41 @@ describe('send(file, options)', function () {
.expect(200, '<p>tobi</p>', done)
})

it('should support function', function (done) {
function index (path) {
var res = this.res

res.end('<p>tobi</p>')
}

var app = http.createServer(function (req, res) {
send(req, req.url, {root: fixtures})
.on('index', index)
.pipe(res)
})

request(app)
.get('/')
.expect(200, '<p>tobi</p>', done)
})

it('should support function as option', function (done) {
function index (path) {
var res = this.res

res.end('<p>tobi</p>')
}

var app = http.createServer(function (req, res) {
send(req, req.url, {root: fixtures, index: index})
.pipe(res)
})

request(app)
.get('/')
.expect(200, '<p>tobi</p>', done)
})

it('should support disabling', function (done) {
request(createServer({root: fixtures, index: false}))
.get('/pets/')
Expand Down