issue 363 #366
issue 363 #366
Conversation
If a pre-existing sourceMappingURL comment is loaded the write process always used `.js` format without a preceding line-feed. This alters the logic so sources with pre-existing sourceMappingURL is written with the correct type of comment for the output file. Fixes #363
There was a problem hiding this comment.
Failing due to 0.8, 0.12 no longer supported by mississipi via
mafintosh/flush-write-stream@5cab12f#r32224180
@phated how should I resolve this for older versions of node for npm. This is easy with yarn, but yarn only works on node>=4
|
|
||
| switch (extension) { |
There was a problem hiding this comment.
got rid of switch statement as I prefer hash / objects.
| break; | ||
| default: | ||
| debug(function() { return 'unknown commentFormatter'; }); | ||
| if (file.sourceMap.preExistingComment) { |
There was a problem hiding this comment.
preExisting now correctly handles css correctly
| @@ -13,7 +13,7 @@ cache: | |||
| - node_modules | |||
|
|
|||
| install: | |||
| - npm i -g npm@4.5.X | |||
There was a problem hiding this comment.
if you just remove the install section, the travis run will use the default npm that comes with each node version. No need to get complicated.
package.json
Outdated
| @@ -55,6 +55,9 @@ | |||
| "vinyl": "2.X", | |||
| "yargs": "7.X" | |||
| }, | |||
| "resolutions": { | |||
| "flush-write-stream": "1.0.3" | |||
There was a problem hiding this comment.
you can just put this in devDeps and it'll be resolved to match the semver range of mississippi.
There was a problem hiding this comment.
reverted the accidental node 0.12 drop in flush-write-stream. fix out in 1.1.1
There was a problem hiding this comment.
Feel free to address my comment about debugging twice or not, it's not that big of a deal.
The only other thought I'm not sure why coverage/* is being checked into git, seems like it should be added to .gitignore. As with my other comment I don't see this as a blocker for merging this.
| } | ||
|
|
||
| debug(function () { |
There was a problem hiding this comment.
It seems like this should be moved into the else {} block just above? Unless you want the !!file.sourceMap.preExistingComment to print two debug messages.
There was a problem hiding this comment.
@coreyfarrell I see you are practaclly the lead on istanbul-api and nyc can you upgrade us to the latest as we're on such an old version of istanbul? Would the new versions still work in node 0.8 and 0.12?
There was a problem hiding this comment.
The current versions of istanbul and nyc all require node.js 6, otherwise I would have done this already.
To be honest I'm hoping that gulp@5 will drop node.js versions that are EOL. The need to avoid dependencies which use ES6 is increasingly difficult.
There was a problem hiding this comment.
100% agree thanks
Coverage is being checked in as it's the baseline to what should be covered. It's why the coverage is failing. The prior baseline was 100% coverage, so essentially it is telling us hey the coverage went down and you need to test more. I'm not going to remove this, if anything I can push up the new baseline with dropped coverage. |
No description provided.