Parse content-type and content-disposition to guess the asset type #454
Conversation
Made github zipball URLs work properly.
Fixed missing ?
file. Updated tar.gz detection to use end of filename rather than match any position in file. Updated test/package.js to check for warning on tar.gz file.
|
I tested this with my projects and it works as expected. |
|
what needs to happen for this to be merged? if someone can tell me, I can try to make it happen (since I seem to be the only one that cares :)). |
|
anything I can do here? |
| @@ -461,8 +461,44 @@ Package.prototype.download = function () { | |||
| return this.emit('error', new Error(res.statusCode + ' status code for ' + this.assetUrl)); | |||
| } | |||
|
|
|||
| var file = fs.createWriteStream(path.join((this.path = tmpPath), 'index' + this.assetType)); | |||
| // Detect correct asset type based on response headers | |||
There was a problem hiding this comment.
This chunk of code does quite a lot of stuff that has a distinct purpose. It should be a separate function that takes res.headers as an argument and returns the assetType.
|
@davidmaxwaterman it needs reviewing by two other core contributors. If you are desperate for the feature you could try using this branch directly rather than waiting for a release that includes it. |
|
@wibblymat I have used the branch directly...that is the problem...I have submitted code to my project that depends on it, and it is taking longer to merge it in than I thought it would. It's only a matter of time before someone tries to build my project and finds it doesn't build properly, and then complains to me :/ I put a dependency on >0.9.3, but there is no such version...yet. |
Asset detection moved to function.
|
@wibblymat @sindresorhus @necolas Can we ship this? I will use puley or rebase. |
| else next(); | ||
| if (this.assetType === '.zip' || this.assetType === '.tar') return this.once('extract', next).extract(); | ||
|
|
||
| if (this.assetType === '.gz') this.emit('warn', 'gz package type not yet supported'); |
There was a problem hiding this comment.
Were we going to move this check elsewhere? It seems to make sense here, I just vaguely remember the discussion.
There was a problem hiding this comment.
I think it's fine to do it there.
|
Added a comment about the (.tar).gz warning but other than that, LGTM. |
|
@wibblymat you added the warning (can't see your commit), or you wanted to say that we should add it? |
|
Oh nvm, when you said "added a comment" you mean that you added a comment here on github lol, I thought it was on the actual code. |
Parse content-type and content-disposition to guess the asset type
This is an improvement over #431 since it also parses the content-type header.
Please review it. Before merging I will rebase it / or use pulley.