add extensions option #65
Conversation
|
@Necromos I will take a look later ((^_^))~ |
|
@coderhaoxin any news? :) |
| @@ -48,6 +48,7 @@ function send(ctx, path, opts) { | |||
| var maxage = opts.maxage || opts.maxAge || 0; | |||
| var hidden = opts.hidden || false; | |||
| var format = opts.format === false ? false : true; | |||
| var extensions = Array.isArray(opts.extensions) ? opts.extensions : false; | |||
There was a problem hiding this comment.
we could check extensions here.
similar to https://github.com/pillarjs/send/blob/master/index.js#L921
There was a problem hiding this comment.
Hmmm, I have seen this approach in express also and it would not be a bad idea to use over here but after a while I'm looping through extensions array so this would add unneeded additional O(n) complexity.
There was a problem hiding this comment.
Yep. But if there is an invalid extension param in extensions option, we should throw an error, not just ignore it silently ?
There was a problem hiding this comment.
Hmmm so what about adding type checker to existing loop? It may look quite '🐟' in code but would preserve complexity.
There was a problem hiding this comment.
what about adding type checker to existing loop
SGTM >_<
| var app = koa(); | ||
|
|
||
| app.use(function *(){ | ||
| yield send(this, 'test/fixtures/hello', {extensions: ['json','htm','html']}); |
There was a problem hiding this comment.
Oh, sorry. Looks like I missed this.
| @@ -462,7 +462,7 @@ describe('send(ctx, file)', function(){ | |||
| var app = koa(); | |||
|
|
|||
| app.use(function *(){ | |||
| yield send(this, 'test/fixtures/hello', {extensions: {}}); | |||
| yield send(this, 'test/fixtures/hello', { extensions: {}}); | |||
There was a problem hiding this comment.
{ extensions: {}} -> { extensions: {} } (^_^)
There was a problem hiding this comment.
Hmmm, looks like I ommited some cases in regex, gonna fix it soon.
There was a problem hiding this comment.
I'm adding refactor of all style issues I found in project but still wondering, why not use some kind of auto-formatter?
| var list = [].concat(extensions || []); | ||
| for (var i = 0; i < list.length; i++) { | ||
| var extension = list[i]; | ||
| if (!(typeof extension === 'string' || extension instanceof String)) { |
There was a problem hiding this comment.
if (typeof extension !== 'string') is enough ((^_^)).
@Necromos I'll merge (rebase) this PR and publish a new version before this weekend.
Great work ! 👍
There was a problem hiding this comment.
@coderhaoxin yup I know but I wanted to keep solution from pillarjs 🐱
Thanks :)
|
@Necromos |
|
@coderhaoxin I will look into it during this week (^_^) |
As mentioned in koajs/static#65 I tried to add extensions option to send.