Relative path manipulation handles Windows paths and different extensions

[chrissewart]

Fixes two problems when combining source maps that already have a relative path.

The first problem is when the extensions differ. e.g. A Typescript or Coffeescript compiler has converted foo.ts into foo.js and something upstream has given them relative paths. bar/foo.ts does not match bar/foo.js so the sources comes out as bar/bar/foo.ts

The second problem is doing the above on Windows. bar\\foo.ts does not match bar/foo.js.

Both occur when running Browserify on Windows on files generated by the Typescript compiler. Most files work ok, but ones that contain node.js globals go through the insert-module-globals module which generates a relative path. The path.join() in combine-source-maps is hit generating a Windows path which then causes the path comparison to fail when this code is hit again during browser-pack

The simplest fix seemed to be to ensure URL path separators everywhere and compare only on dirname.

Tests added for both problems.

[chrissewart]

Required to make existing tests pass on Windows, but probably wise anyway.

[brettz9]

This is really a big problem since global detection is on by default in browserify and code built on Windows with browserify (which relies on combine-source-map) will also fail for all other users for source files that contains references to global, etc..

Is this library still being maintained? If not, I think we ought to ask browserify to require the fork in this PR.

[thlorenz]

please fix the alignment of || here, they were all in the same column for improved readability.
Also cut the comment a bit shorter, (remove the e.g. part).

[thlorenz]

@brettz9 still maintained, just short on time.
I added a comment of a nit that needs fixing.
Do you want to take on fixing this and then merging to master so I can publish @brettz9?
If so I'll make you a collaborator.

[brettz9]

Sure, I'll be happy to adjust the PR. Re: the alignment, would you like me to break after the === and indent the next line then, adding spaces as necessary to align the ||?

However, while the backslashes issue is clear to me (and the main reason for my asking for the merge), and while I understand the rationale for the dirname comparison in the PR, I wonder--if I am understanding the situation correctly--whether more caution is in order in ensuring that the file names are not only in the same (perceived) directory but are also the same pre-extension (e.g., foo/abc.coffee and foo/abc.js are more likely related but with foo/abc.coffee and foo/def.coffee, the latter could presumably also have been foo/foo/def.coffee). Even as it is now, I imagine the identity check you have could with say 'foo/abc.js' === 'foo/abc.js' give a false match if there were both a foo/abc.js and also a foo/foo/abc.js though unless a strict option were added, I'd think this would be the usually desired behavior. OTOH, maybe leaving it as is is also ok since there may be cases of foo/abc.coffee and foo/abc-converted.js.

Sorry if I'm muddling things based on an incomplete understanding but it actually seems like, except for the backslashes issue, that it ought to be possible to supply more information to addFile when the sourceFile path is relative (like its own sourceRoot?) so the comparisons will always be fully precise when paths are not absolute and there is no need for making imperfect guesses at adjusting the paths.

[chrissewart]

IIRC, I went through much the same thinking and chose a simple dirname comparison for two reasons. Firstly, as you say, there may be cases of foo/abc.coffee and foo/abc-converted.js (although I can't think of any). Secondly, it looked like sourcePath and relativePath function args were supposed to refer to the 'same thing' because when the condition fails, they get combined in line 49....but I may have misunderstood the context this is being called in.

Apologies for ruining the layout. Looking back, adding a function isSameDirectory() would make the line shorter and most of the comment redundant. The // e.g. foo.ts > foo.js seems more important to keep because it explains why the line is there, rather than simply what it does.

[brettz9]

Although I think this is sound, I would like to take another look when I get a chance at how brower-pack and insert-module-globals are making their calls and see whether part of the problem in browserify is on their end in how they supply the map data for combination.

But the backslash issue at least ought to be fixed here and the other items in the PR should still be valid, particularly if all path info needed for a good comparison is being and can be supplied.

[brettz9]

@thlorenz : I'm not sure if or when I may have time to debug things on the browserify side or to better contemplate the soundness of directory comparisons here.

It does seem clear that combine-source-map ought to be used by consuming libraries in comparing apples to apples (per @chrissewart 's comment, "sourcePath and relativePath function args were supposed to refer to the 'same thing'"). However, I'm unsure whether the shortcircuiting in the case of same directories will inadvertently avoid rebasing unrelated files in the same directory. Maybe Chris can comment on this?

If you don't have time to consider it either, Thorsten, I suggest at least merging #21 which is a no-brainer for Windows.

[brettz9]

In giving this a little further thought, it seems to me that @chrissewart's changes are not only generally safe but that we ought to go further in avoiding the assumption that existing map sources should be rebased relative to newly added files (unless some config is passed in to indicate such a desire). Is there some compelling reason why this assumption is being made? (If such cases are required, I'd be interested to know whether browser-pack and browserify ought to be making use of them by default.)

[brettz9]

But if there is a need for rebasing by default, it seems to me that making exceptions for source file conversions like CoffeeScript/TypeScript ought to be specified via explicit config in some manner. In looking at coffeeify, I see a file property is set with a JS extension distinct from the Coffeescript source files. Maybe the file property ought to be compared instead? (Or for browserify itself, leveraging info during the deduping phase as deduping sets the likes of nomap, causing browser-pack to avoid adding files via combine-source-map; maybe CoffeeScript or other such transforms could make calls that flag items to avoid them being treated as genuinely new items to be added?)

I hate to be feeling around in the dark here, but I am hoping this may provide some food for thought by someone more comfortable with the code base, source map specs, and the consumings apps.

[thlorenz]

Do we still need this @brettz9 given we applied the two other changes?

[brettz9]

There is still an aspect of the PR not incorporated which ought to be addressed somewhere (namely ensuring that pre-compiled sources like bar/foo.ts when compared to the same file post-compilation, bar/foo.js, does not get rebased into bar/bar/foo.ts when no equivalence is found between the files).

However, unless @chrissewart might provide further rationales, my previous two comments attempted to address why I am not convinced that the still-unincorporated aspect of this PR, namely, this line:

path.dirname(sourceFile) === path.dirname(relativeRootedPath) || // same dir, different file (e.g. foo.ts > foo.js)

...while understandable and solving the problem, doesn't strike me as a fully satisfying solution.

[thlorenz]

OK, thanks for the feedback @brettz9.
In that case we'll wait for @chrissewart to reply and clarify things.

If we don't hear from him in a month or so we'll just close the PR.

[chrissewart]

My rationale for the contentious change referenced by @brettz9 above is where some upstream module has already done a mapping and given the result a relative path. Specifically, modules containing node globals go through insert-module-globals which generates a relative path. Without the change rebaseRelativePath() mangles the perfectly ok mapping as per the original comment.

So, anything that wasn't originally JS that uses node globals breaks.

My reasoning that ignoring the filename is OK is that if the conditional isn't true the resulting code also ignores the filename - it plucks it off sourceFile and uses the one from relativePath !!

Given the debate it has caused, I can see that my proposed solution isn't fully satisfying, but figured it was better than some other person spending the hours I did figuring out why some of their project's TS files couldn't be debugged with sourcemaps. :-)

If I was to try for something better, I'd note that

• The history of this function seems to be to add more and more cases to the conditional to stop the last line from mangling things.
• The test case makes a good case for why the function was added.
• A side effect is that when a compile-to-js step is used relativePath is usually just foo.ts (no path) but sourceFile is bar/foo.js, the return value is the correct bar/foo.ts
• Another side effect is that URL separators are always returned, even on Windows.
• Finally, the initial if (!relativePath) returns null which the calling code passes to inline-source-map which has no obvious/stated handling for null mappings.

So, in my head, it should work like this:

function makeRelative(sourceFile, relativeSourceRoot, relativePath) {
  if (!relativePath) {
    return sourceFile;  // Can't do anything 
  }

  if (relativeSourceRoot) {
    // Handles the case where we are combining bundles that already contain mappings.  
    // @see test 'relative path from multiple files' 
    return path.join(path.dirname(sourceFile), relativeSourceRoot, relativePath);
  }

  if(path.dirname(relativePath) === '.') {
    // relativePath is just a file name, make it actually relative. 
    // Handles compile-to-js builds where the output is often in the same dir as the input. 
    return path.join(path.dirname(sourceFile), relativePath);
  }

  // Something upstream has already worked it out. 
  return relativePath; 
}

function toUrlPathSeparator(path) { 
  return path.replace(/\\/g, '/');
}

function fixPath(sourceFile, relativeSourceRoot, relativePath) {
  return toUrlPathSeparator(makeRelative(sourceFile, relativeSourceRoot, relativePath))
}

But right now I can't even run the tests.