Change sourceRoot resolution to match the spec #286
Conversation
|
This allows removing a bunch of code from devtools-source-map as well. |
tromey
force-pushed
the
fix-source-root-prepending
branch
from
c7992c5 to
43f5980
Compare
|
Rebased, and avoid |
gregtatum
left a comment
gregtatum
left a comment
There was a problem hiding this comment.
The logic looks good as far as I can tell. I had an optional comment on some more unit tests and on a few more explanatory comments.
|
|
||
| if (sourceRoot) { | ||
| // This follows what Chrome does. | ||
| if (sourceRoot[sourceRoot.length - 1] !== '/' && sourceURL[0] !== '/') { |
There was a problem hiding this comment.
I agree that we should do what Chrome does here, the spec doesn't seem to define this area very well :-/
| sourceURL = sourceRoot + sourceURL; | ||
| } | ||
|
|
||
| if (sourceMapURL) { |
There was a problem hiding this comment.
I would add a few more comments here explaining why you are doing this work, and how it conforms to the spec. It feels a bit mysterious to me.
| } | ||
|
|
||
| return normalize(sourceURL); | ||
| } |
There was a problem hiding this comment.
This function drives a lot of the internals, and is dependent on behavior both defined from the spec, and implicitly derived from other's implementations. This code gets hit by our existing tests, but I think it warrants having unit tests just for this function that run through the various types of options that can be done here. I took some time to verify my assumptions in the console of how things worked in order to have more confidence in this review. I think some tests could be fairly quick and terse, but would really help in defining the desired behavior here.
assert.equal(
computeSourceURL("http://correct.com/", "/a/b.js", "http://wrong.com/source.map"),
"http://correct.com/a/b.js"
)What do you think?
There was a problem hiding this comment.
That's a great idea.
| 'sources': ['original.js'], | ||
| 'names': [], | ||
| 'mappings': 'AACA', | ||
| 'sourcesContent': ['yellow warbler'] |
There was a problem hiding this comment.
Your birds are starting to compete with fitzgen's hip hop references.
The source-map library has long implemented sourceRoot resolution using
URL resolution. However, the source map specification is reasonably
clear that concatenation is to be used instead.
There is more information in these bugs:
mozilla#62
https://bugzilla.mozilla.org/show_bug.cgi?id=977463
This patch changes the resolution to match what Chrome does. This is an
incompatible change; but, I believe, better for interoperability.
This patch also adds an optional sourceMapURL parameter to the
SourceMapConsumer constructor. This allows us to implement the full
source URL resolution algorithm in the source-map library.
Fixes mozilla#62
tromey
force-pushed
the
fix-source-root-prepending
branch
from
43f5980 to
54825b8
Compare
2 participants
The source-map library has long implemented sourceRoot resolution using
URL resolution. However, the source map specification is reasonably
clear that concatenation is to be used instead.
There is more information in these bugs:
#62
https://bugzilla.mozilla.org/show_bug.cgi?id=977463
This patch changes the resolution to match what Chrome does. This is an
incompatible change; but, I believe, better for interoperability.
This patch also adds an optional sourceMapURL parameter to the
SourceMapConsumer constructor. This allows us to implement the full
source URL resolution algorithm in the source-map library.
Fixes #62