Bug in getting embedded source contents #199
Conversation
fitzgen
force-pushed
the
source-content-bug
branch
from
de03c6c
to
bb38932
Compare
|
Ok, figured this out. We rely on maintaining the invariant that sources are internally stored relative to the source root (except when the source is absolute and the source root is not) and there is a case where we weren't maintaining this: when the source is absolute and the source root is a prefix of the source. Why would a tool do that? No idea. Probably not on purpose.
|
|
Oops jryans is on pto. I just chose him because he had dealt with source root issues before but it doesn't need his review in particular.
|
|
Everyone is on PTO...
|
| // be particularly problematic when the source root is a prefix of the | ||
| // source (valid, but why??). See github issue #199 and bugzil.la/1188982. | ||
| .map(function (source) { | ||
| return sourceRoot && sourceRoot.charAt(0) === '/' && source.charAt(0) === '/' |
There was a problem hiding this comment.
I thought most sourceRoots were URLs, not simple local paths? Do we need a better check for "is absolute" to cover those too?
There was a problem hiding this comment.
I got halfway through writing a comment about why I didn't think so, but then unconvinced myself and finally realized that we should just add test coverage for more edge cases here...
fitzgen
force-pushed
the
source-content-bug
branch
from
bb38932
to
3bc745b
Compare
|
Ok added the test for URLs too. r? @jryans |
| @@ -185,6 +185,10 @@ define(function (require, exports, module) { | |||
| } | |||
| exports.join = join; | |||
|
|
|||
| exports.isAbsolute = function (aPath) { | |||
There was a problem hiding this comment.
Should normalize use this new utility instead of having it own similar check?
|
r+, looks good overall! |
fitzgen
force-pushed
the
source-content-bug
branch
from
3bc745b
to
4951ebf
Compare
Bug in getting embedded source contents
This is just a failing test case right now. Haven't dug into it yet.
See also https://bugzilla.mozilla.org/show_bug.cgi?id=1188697