Sourcemap sourceRoot #1141
Sourcemap sourceRoot #1141
Conversation
src/packagers/SourceMapPackager.js
Outdated
| @@ -16,7 +16,7 @@ class SourceMapPackager extends Packager { | |||
|
|
|||
| async end() { | |||
| let file = path.basename(this.bundle.name); | |||
| await this.write(this.sourceMap.stringify(file)); | |||
| await this.write(this.sourceMap.stringify(file, this.options.rootDir)); | |||
There was a problem hiding this comment.
Should the sourceRoot be a relative path rather than absolute? seems like maybe path.relative(options.outDir, options.rootDir)?
There was a problem hiding this comment.
Updated to relative from outDir path
DeMoorJasper
changed the title
| ); | ||
| assert( | ||
| fs.existsSync( | ||
| path.resolve( |
There was a problem hiding this comment.
This is not a sufficient test, because it passes as long as path.resolve() can resolve it. But sourceRoot is a URL, so you need to test that resolving it as URL results in the right location. Which it currently doesn't, because sourceRoot doesn't contain a trailing slash, and the spec says sourceRoot gets prepended to each source (without magically adding a slash in between as path.resolve() will do).
There was a problem hiding this comment.
See the source map spec:
Line 4: An optional source root, useful for relocating source files on a server or removing repeated values in the “sources” entry. This value is prepended to the individual entries in the “source” field.
Resolving Sources
If the sources are not absolute URLs after prepending of the “sourceRoot”, the sources are resolved relative to the SourceMap (like resolving script src in a html document).
| let mapObject = JSON.parse(map); | ||
| assert( | ||
| mapObject.sourceRoot === | ||
| path.relative(b.options.outDir, b.options.rootDir), |
There was a problem hiding this comment.
This test is incorrect on Windows because sourceRoot should always be a URL with forward slashes, but on Windows this will assert that it's a path with backslashes
|
@felixfbecker Please open a new issue, otherwise this will be forgotten. |
Closes #1100 Closes #1078