Fix paths in sourcemaps

[faergeek]

Now sourcemap paths are relative to cwd (or gulpfile.js) and for external sourcemaps and includeContent: false it's useless.
Here is the same fix as applied in gulp-less.

[jescalan]

Great catch. I think this should definitely be merged, only thing I'd say is maybe move the added functionality out to a function? This would make the logic a little easier to decipher.

[faergeek]

What about something like this:

makePathsRelative(file.base, res.sourcemap.sources);

function makePathsRelative(base, paths) {
  for (var i = 0; i < paths.length; i++) {
    paths[i] = path.relative(base, paths[i]);
  }
}

Or this:

var makePathRelative = path.relative.bind(null, file.base);
res.sourcemap.sources = res.sourcemap.sources.map(makePathRelative);

Or even combo:

res.sourcemap.sources = makePathsRelative(file.base, res.sourcemap.sources);

function makePathsRelative(base, paths) {
  var makePathRelative = path.relative.bind(null, base);
  return paths.map(makePathRelative);
}

Or just oneliner:

res.sourcemap.sources = res.sourcemap.sources.map(path.relative.bind(null, file.base));

I can't decide what is more readable.

[jescalan]

I would say the first one strikes me as most readable. It also follows the same form as vinyl-sourcemaps-apply, which is right below it, for continuity. Maybe you could have the first arg just be file to match it even further 😀

[stephenlacy]

@faergeek the first one is best, please declare it to a variable as an anonymous function.

[jescalan]

I'd much prefer a named function actually, as that way you could place it at the bottom of the file and it would be hoisted. Named functions are entirely legitimate and safe, and often make code more readable 😀

[faergeek]

Check this out

[stephenlacy]

I like that, though I do not like to use named functions...

[jescalan]

That looks sexy as hell @faergeek. 👍 for merge from me!

[jescalan]

Also happy to make a case for why named functions are better if you want to hear my opinion on it @stevelacy -- there are a number of very good reasons 😀

[faergeek]

@stevelacy what's wrong with named functions?

[stephenlacy]

@Jenius and I discussed the functions conventions.

[revyh]

How about to make this functionality conditional?

My project have next structure:

project
|-src
  |-styles
    |-main.styl
|-node_modules
  |-normalize.css
    |-normalize.css

and gulp config is:

var
  gulp = require('gulp'),
  stylus = require('gulp-stylus'),
  sourcemaps = require('gulp-sourcemaps');

gulp.task('styles', function () {
  gulp.src('src/styles/styles.styl')
    .pipe(sourcemaps.init())
    .pipe(stylus({
      'include css': true,
      import: require.resolve('normalize.css'),
    }))
    .pipe(sourcemaps.write())
    .pipe(gulp.dest('dst'));
});

So, after stylus rendering sourcemap looks like this:

{
  "sources": [
    "node_modules/normalize.css/normalize.css",
    "src/styles/styles.styl"
  ],
  "file": "styles.css"
  ...
}

And, after makePathRelative():

{
  "sources": [
    "../../node_modules/normalize.css/normalize.css",
    "styles.styl"
  ],
  "file": "styles.css"
  ...
}

At the end of it all, gulp-sourcemaps adds sourceRoot property:

{
  "sources": [
    "../../node_modules/normalize.css/normalize.css",
    "styles.styl"
  ],
  "file": "styles.css"
  "sourceRoot": "/sources/"
  ...
}

In this way, for normalize.css we have path /sources/../../node_modules/normalize.css/normalize.css which is out of server root. Chromium devtools (44.0.2403.89 Ubuntu) can fight it somehow (I assume, that Chromium doesn't care about it, because sources are inlined), but Firefox(42.0a2) refuse to show sources at all.

So, I propose to apply makePathsRelative() if:

1. there are no inline sources, because it doesn't make difference
2. there is no sourceRoot, because it break browser path resolving

[faergeek]

@revyh Can you provide some minimal example to test against?

[revyh]

@faergeek here is example repo

[revyh]

@faergeek At the moment, I doubt that we really need makePathsRelative() at all.

Here, I'll try cover all possible ways of using sourcemaps:

• original sources hosted separately of sourcemap In this case, user needs to host original sources and browser needs to load them.
  • There is sourceRoot in sourcemap. Browser will resolve sources relatively to sourceRoot to locate original sources. If we change sources, it may break browser resolving.
  • There is no sourceRoot in sourcemap. Browser will resolve sources relatively to "sourcemap location".
    • sourcemap is a standalone JSON file. Then "sourcemap location" is location of this JSON file. We don't know JSON file location, because it will be determined later on a gulp pipe, when we call gulp-sourcemaps.write('JSON_file_location') So again, changes in sources may break browser resolving.
    • sourcemap inlined as base64 dataURI in comment in compiled/bundled file. Then "sourcemap location" is location of this bundled file. I think, that in makePathsRelative() you assumed, that file.base is a bundled file location. But, it isn't true, because file.base is a base of original source file. Furthermore, file properties (like base, relative etc) may be changed by gulp-rename or other tool down the pipe. So, we don't know bundled file location and still can break browser resolving by changing sources. However, makePathsRealtive() can work in simple cases like gulp.src('original_src_file.styl').pipe(gulp-stylus()).pipe(gulp.dest('web_server_root');
• Original sources inlined to sourcemap. In this case, user doesn't need to host original sources and browser doesn't need use sources to locate this original sources. So, we'd better leave sources alone.

In general, sources - is only one part of browser resolving mechanic, so if we change this part, then

1. we can break browser resolving
2. we can frustrate user with unexpected plugin behavior
3. we limit users control over their code

As I think, makePathsRelative() goals can be achieved with native sourcemap option for stylus, like sourcemap.basePath. But, for now, we just follow the instructions in gulp-sourcemap and override user-defined properties:

if (file.sourceMap) {
  opts.sourcemap = true;
}
...
stylus.render(file.contents.toString('utf8'), opts)

Correct me if I'm wrong.

[faergeek]

@revyh I partly agree with you, but I checked another example and can't agree that we do not need makePathsRelative at all.
I will provide that example a little later, after reducing it.
For now you can fix it if you set sourceRoot to __dirname.
I will investigate further.

[faergeek]

I see it as a combination of these two programming problems for now :-):

[faergeek]

Correct me if I'm wrong, but I think the only right solution here is to provide sourceRoot to sourcemaps.write call when includeContent is true (the default).

The problem is that when includeContent is true sources shown relative to virtual source folder and when it sees path like ../../ it just goes up and do not find sources there.

So it depends on depths of your folder structure for source and destination plus includeContent value.
I can't determine the value of includeContent from gulp plugin.

@stevelacy @Jenius thoughts?

[faergeek]

Here's my test case
https://github.com/faergeek/gulp-stylus-testcase

[revyh]

@faergeek Yeah, the picture is old, but still funny 😄

tl;dr. For now I think that my issue was because of FF bug, but I still don't think that we need makePathsRelative

The problem is that when includeContent is true sources shown relative to virtual source folder and when it sees path like ../../ it just goes up and do not find sources there.

Well, I don't think it must works like this. When includeContent is true stylus just stringify and copy content of source files in sourcemap sourcesContent array. In this situation, when browser is trying to show source code in devtools, it doesn't need to physically load any files, it only needs to read sourcesContent array. So, any errors in sources paths must be ignored. But FF don't ignores and I think it is its bug.

I tried several combinations of sourceRoot and sources for includeContent === true to find out which works and which doesn't. So, table below shows my results. First row - are variants of the sourceRoot and first column - are variants of the sources. Other cells contain assumed resulted path for col/row combination and 👌 if FF working or 💩 if it is not. For simplicity, I assume that sourcemap file located at /A/main.css.map (which in reality may be something like /home/user_name/Development/project/...) and protocol is file://

	none	'/'	'/blah'	'.blah'
'main.styl'	👌'/A/main.styl'	👌'/main.styl'	👌'/blah/main.styl'	👌'/A/main.styl'?
'./main.styl'	👌'/A/main.styl'	👌'/main.styl'	👌'/blah/main.styl'	👌'/A/main.styl'?
'../main.styl'	💩'/main.styl'	💩out of'/'	💩'/main.styl'	💩'/main.styl'?
'../A/main.styl'	💩'/A/main.styl'	💩out of'/'	💩'/A/main.styl'	💩'/A/main.styl'?
'/main.styl	👌'/main.styl'	👌'/main.styl'	👌'/main.styl'	👌'/main.styl'

When sourceRoot is relative path (.blah) it is resolved against sourcemap file (/A/B/main.css.map) parent folder. Looks like, since /A/blah doesn't exist, sourceRoot in last column is set to /A (sourcemap file paren folder), which is the same as if there was sourceRoot missed (none).

Now the most interesting part. It looks like FF doesn't work if sources starts with ../ and resolving this ../ goes up to root folder /. For example, FF would work if sourceRoot is /blah/blah and sources start with ../ (../main.styl). But FF refuse to work, when sources starts with ../../, no matter what goes after ../../ (../../main.styl or ../../A/B/C/main.styl). At the same time, FF works fine with other relative paths in sources (./main.styl or main.styl), even if sourceRoot is root folder /. Also FF works well with absolute paths in sources (/main.styl). I think it may be related to this issue. Last one interesting thing, is that if sourceRoot is a too long ../../../../../../../../../../../../, then FF works with sources like main.styl or ./main.styl, but not with ../main.styl.

This problem can be solved many ways, for example with mold-source-map. @faergeek's solution will work as long as the sourceRoot + souces reaches root folder / with help of ../.

As for makePathsRelative. When I need to change sourceRoot according to makePathRelative, it makes me feel like I fix something that ain't broken. So it would be interesting to know, what goals have forced @faergeek to do this PR.

And I agree with @faergeek: It would be great to hear @stevelacy or @Jenius thoughts.

[stephenlacy]

@revyh @faergeek Thanks for debugging!

I played with both test cases, with screenshots for reference:

@revyh's case:

Top left: Chrome Without makePathsRelative
Right: Firefox without makePathsRelative
Bottom left: Chrome with makePathsRelative

Removing makePathsRelative does keep a more organized folder structure.

@faergeek's case:

Top left: Chrome
Top right: Firefox

I am considering adding a gulp-stylus option relativePaths: Boolean, thoughts?

That way it would be false be default, enabling it would then change the sources array with path.relative; Instead of adding a sourceRoot just to override makePathsRelative.

.pipe(stylus({
  relativePaths: true
}))

[faergeek]

@stevelacy Yes, option seems like the only solution for now.
May be use the name like relativeSourcePaths or relativeSources or something like that?

[revyh]

@stevelacy @faergeek Sorry for delay. I don't think we need a new option. I created demo to illustrate it. In this demo gulp default task creates three example page:

• makePathsRelative - with using current gulp-stylus implementation
• basePathRelative - with using native stylus API and option sourcemap: {basePath: file.base}
• notRelative - with using native stylus API and default basePath value

makePathsRelative and basePathRelative are identical. So, we can replace makePathsRelative with setting sourcemap: {basePath: file.base} by default. In this way (unlike makePathsRelative), users can overwrite this default basePath value with explicitly declared one and get something like notRelative (which solves my issue).

I think that basePath better than proposed relativePaths option, because basePath native and more tuneable (String instead of Boolean). Moreover, it would be great if basePath isn't setted to file.base by default and users had to specify it explicitly to achieve makePathsRelative behavior. It's because makePathsRelative changes default stylus behavior, and I believe that wrapper plugins (like this one) must be as transparent as possible. But I don't expect you agree with me because of backward compatibility.

Anyway, in order to use basePath we need to stop rewrite sourcemap option as I mention before.

[revyh]

Hello again, guys.

It's been two weeks and no feedback from you. Maybe you don't want to solve this problem or you don't have enough time for it, or the problem is paltry? Or maybe you are waiting for some action from me (make PR or open an issue, or leave you alone)? Whatever it was, I just want to know.

[stephenlacy]

@faergeek see any issues with base paths?

[faergeek]

@stevelacy I'm not sure if it won't break anything.
I even don't know exactly in which cases output will differ and we'll got a regression.

So anyway would be better to write some tests for sourcemaps, try to use base paths and bump major release after that.

But actually I'm not sure if I could do that. I don't use gulp in my projects now, sorry.

@revyh Would you like to submit such pull request?