Fix missing path normalization, causing empty sourcesContent in source maps.

[thomas-darling]

If source maps already exist for the files being bundled, and if those source maps have their "sourceRoot" set to anything other than an empty string (such as "/" or "../"), then the builder fails to include the original source content in the source map for the bundle.

This happens because line 48 in sourcemaps.js fails to take into account that the source path may already be absolute, or if is relative, that it needs to be normalized.

This lack of normalization becomes a problem in line 85, where the source map itself has been normalized, while the keys for which the source content were stored are still the non-normalized paths - and therefore, the original content is not found. When it then tries to load the content from the original source file, it fails again if the original source maps had its "sourceRoot" set to an absolute path, as map.sourceRoot would then have been incorrectly prepended to the path.

This pull request fixes this, so if the path is already absolute, it is used as-is, and and if it is relative, it is normalized. This means source content is now correctly loaded from existing sourcemaps and included in the source map for the bundle - or if the original source maps do not contain the content, correctly loaded from the original source files.

[asapach]

@thomas-darling maybe you could add a test or two similar to this one? https://github.com/systemjs/builder/blob/master/test/sourcemaps.js#L97-L101

[thomas-darling]

@asapach it would definitely be good to have tests for this, but I can't really afford to spend any more time on this right now, unfortunately. So for now, this will have to do - I'll keep it in mind for later though, as we have a pretty strong interest in making sure this stuff works correctly.

By the way, my second commit here ensures it checks correctly for absolute paths, and resolves an issue where absolute source paths were incorrectly changed to relative paths in the source map for the bundle. Now absolute source paths are preserved without changes - the behavior for relative paths is unchanged.

[guybedford]

This may have caused an issue here - jspm/jspm-cli#2361.

[guybedford]

There seem to be more reports coming in there. I just don't have the time to debug source maps right now in my life. If anyone is able to help that would be great, otherwise I will just revert if there are more issues.

[thomas-darling]

Well, it makes no sense to revert it, as it fixes some pretty serious bugs.

I think the issue here is pretty clear - after the change, we are preserving absolute paths, which means that paths starting with /, \ or C:\... are preserved, where they were previously made relative.

I'd argue this is the correct behavior, and that if you don't want absolute paths in source maps, then you should not create them with absolute paths when building the code in the first place :-)

Also, I'm willing to bet that many people switched to absolute file system paths because the relative paths were broken before - we were down that road too.

Now, if you want to revert to the old behavior for absolute paths rooted in a drive letter, then all we have to do is to replace that last path.isAbsolute on line 101 with a more explicit check for a leading / or \ :-)

[guybedford]

There may be some steps in the conceptual model where the individual intermediate source maps by the compilers use absolute paths as the intermediate processing step, before being made relative at the final concatenation step. See https://github.com/systemjs/builder/blob/master/compilers/compiler.js#L20.

This was done so that source maps can be concatenated at the final step without having to renormalize each one individually first.

The issue might then be to make this absolute treatment in this PR compatible with that. Let me know if that sounds like the right sort of assessment, I may be completely off here too.

[guybedford]

That is, file:///path/to/x.js is used in the intermediate representation, although it doesn't look like that would be caught by the isAbsolute here actually at all. If so, then it may just be the actual absolute cases that are failing correctly.

[thomas-darling]

Yeah, I can't claim to have a full understanding of every possible code path that might lead to this function, but based on my fairly extensive debugging of the original issue, I'm reasonably sure just changing that isAbsolute to a more explicit check would do the trick - because as you say, the file:// paths were never affected by the changes :-)

I guess the question is whether this should be considered a bug at all though? As noted before, this now preserves the original, absolute sources, which is what I would personally expect.

I think the issue people are facing might be, that Chrome can't load source maps from just a file system path - previously such paths were made relative, and that's why it kind of worked, although I could imagine it might sometimes behaved in unexpected ways. If we absolutely have to do something about this, then maybe the best way to resolve it, would be to simply ensure absolute paths rooted in a drive letter are always turned into file:// paths, which I believe Chrome can load.

Alternatively, I would not object to just just making that isAbsolute check more explicit... I don't really see any valid reason to use absolute paths rooted in a drive letter anyway, so whatever works for those who prefer that... :-)

Thoughts?