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.

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

fix: address file URL path regression on Windows #146

Merged
merged 3 commits into from Jun 3, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/v8-to-istanbul.js
@@ -1,6 +1,7 @@
const assert = require('assert')
const convertSourceMap = require('convert-source-map')
const { dirname, isAbsolute, join, resolve } = require('path')
const { fileURLToPath } = require('url')
const CovBranch = require('./branch')
const CovFunction = require('./function')
const CovSource = require('./source')
Expand Down Expand Up @@ -83,7 +84,10 @@ module.exports = class V8ToIstanbul {
}

_resolveSource (rawSourceMap, sourcePath) {
sourcePath = sourcePath.replace(/(^file:\/\/)|(^webpack:\/\/)/, '')
if (sourcePath.startsWith('file://')) {
return fileURLToPath(sourcePath)
}
sourcePath = sourcePath.replace(/^webpack:\/\//, '')
const sourceRoot = rawSourceMap.sourcemap.sourceRoot ? rawSourceMap.sourcemap.sourceRoot.replace('file://', '') : ''
const candidatePath = join(sourceRoot, sourcePath)

Expand Down Expand Up @@ -282,5 +286,5 @@ module.exports = class V8ToIstanbul {
}

function parsePath (scriptPath) {
return scriptPath.replace('file://', '')
return scriptPath.startsWith('file://') ? fileURLToPath(scriptPath) : scriptPath
}
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -38,7 +38,7 @@
"tap": "^14.10.8"
},
"engines": {
"node": ">=10.10.0"
"node": ">=10.12.0"
Copy link
Member

Choose a reason for hiding this comment

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

10 is EOL, just drop it entirely and only support 12+ since we already need to do a major?

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 don't mind either way, but seems like @bcoe still needs Node 10 support. 🤷‍♂️ See #143 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

hah. my condolences 😀

(that said: jestjs/jest#11167 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to go ahead and land this as a breaking change, I'm fine with the requirement for Node 10.12, and it means my team can keep using c8 until we drop Node 10 (hopefully in the next couple months).

},
"files": [
"lib/*.js",
Expand Down
21 changes: 11 additions & 10 deletions test/v8-to-istanbul.js
@@ -1,6 +1,7 @@
/* global describe, it, beforeEach, afterEach */
const { readdirSync, lstatSync, writeFileSync, readFileSync } = require('fs')
const path = require('path')
const { pathToFileURL } = require('url')
const runFixture = require('./utils/run-fixture')
const V8ToIstanbul = require('../lib/v8-to-istanbul')
const crypto = require('crypto')
Expand All @@ -25,7 +26,7 @@ describe('V8ToIstanbul', async () => {

it('handles ESM style paths', async () => {
const v8ToIstanbul = new V8ToIstanbul(
`file://${require.resolve('./fixtures/scripts/functions.js')}`,
pathToFileURL(require.resolve('./fixtures/scripts/functions.js')).href,
0
)
await v8ToIstanbul.load()
Expand Down Expand Up @@ -94,7 +95,7 @@ ${'//'}${'#'} sourceMappingURL=data:application/json;base64,${base64Sourcemap}

it('should clamp line source column >= 0', async () => {
const v8ToIstanbul = new V8ToIstanbul(
`file://${require.resolve('./fixtures/scripts/needs-compile.compiled.js')}`,
pathToFileURL(require.resolve('./fixtures/scripts/needs-compile.compiled.js')).href,
0
)

Expand Down Expand Up @@ -124,7 +125,7 @@ ${'//'}${'#'} sourceMappingURL=data:application/json;base64,${base64Sourcemap}

it('should exclude files when passing excludePath', async () => {
const v8ToIstanbul = new V8ToIstanbul(
`file://${require.resolve('./fixtures/scripts/sourcemap-multisource.js')}`,
pathToFileURL(require.resolve('./fixtures/scripts/sourcemap-multisource.js')).href,
0,
undefined,
path => path.indexOf('bootstrap') > -1
Expand All @@ -137,7 +138,7 @@ ${'//'}${'#'} sourceMappingURL=data:application/json;base64,${base64Sourcemap}
endOffset: 1
}]
}])
Object.keys(v8ToIstanbul.toIstanbul()).should.eql(['/src/index.ts', '/src/utils.ts'])
Object.keys(v8ToIstanbul.toIstanbul()).should.eql(['/src/index.ts', '/src/utils.ts'].map(path.normalize))
})
})

Expand All @@ -152,30 +153,30 @@ ${'//'}${'#'} sourceMappingURL=data:application/json;base64,${base64Sourcemap}
})
it('should handle empty sources in a sourcemap', async () => {
const v8ToIstanbul = new V8ToIstanbul(
`file://${require.resolve('./fixtures/scripts/empty.compiled.js')}`,
pathToFileURL(require.resolve('./fixtures/scripts/empty.compiled.js')).href,
0
)
await v8ToIstanbul.load()
})

it('should handle relative sourceRoots correctly', async () => {
const v8ToIstanbul = new V8ToIstanbul(
`file://${require.resolve('./fixtures/scripts/relative-source-root.compiled.js')}`,
pathToFileURL(require.resolve('./fixtures/scripts/relative-source-root.compiled.js')).href,
0
)
await v8ToIstanbul.load()
assert(v8ToIstanbul.path.includes('v8-to-istanbul/test/fixtures/one-up/relative-source-root.js'))
assert(v8ToIstanbul.path.includes(path.normalize('v8-to-istanbul/test/fixtures/one-up/relative-source-root.js')))
})

it('should handles source maps with moultiple sources', async () => {
it('should handles source maps with multiple sources', async () => {
const v8ToIstanbul = new V8ToIstanbul(
`file://${require.resolve('./fixtures/scripts/sourcemap-multisource.js')}`,
pathToFileURL(require.resolve('./fixtures/scripts/sourcemap-multisource.js')).href,
0
)
await v8ToIstanbul.load()

v8ToIstanbul.covSources.length.should.equal(3)
Object.keys(v8ToIstanbul.toIstanbul()).should.eql(['/webpack/bootstrap', '/src/index.ts', '/src/utils.ts'])
Object.keys(v8ToIstanbul.toIstanbul()).should.eql(['/webpack/bootstrap', '/src/index.ts', '/src/utils.ts'].map(path.normalize))
})
})

Expand Down