Improve source maps API

[lydell]

I have some ideas for the source maps API that will:

• Clean the options up a bit.
• Nicely add requested features.
• Allow better source path handling.
• Still not make too much difference for the user.
• Preserve all current default behavior.

I propose:

• Enhance options.map:
  • If omitted, default to false. (Like now.)
  • false → don’t generate source map. (Like now.)
  • true → equivalent to {}.
  • Deprecate passing previous map, such as map: sassMap. But allow it until it is removed.
  • If an object: Generate source map. Options:
    • map.annotation:
      • If omitted, default to 'preserve'.
      • 'preserve' → Use same annotation type as input. (Like mapAnnotation: true now.)
      • 'none' → No annotation. (Like mapAnnotation: false now.)
      • 'inline' → Inline the source map into the annotation. (Like inlineMap: true now.)
      • 'path' → Use map.path as annotation. (New feature, though kind of possible now, too, at least partly…)
    • map.path:
      • If omitted, default to options.to + '.map'. (This is actually the current behavior, more or less: We always assume that the source map will either be saved next to the CSS, or inlined into the CSS.)
      • It is the path to the intended location where the source map will be written to disk. This allows to store the source map somewhere else than only next to the CSS or inlined into the CSS, and still have correct source paths. (New feature, inspired by https://github.com/reworkcss/css-stringify/issues/39/https://github.com/reworkcss/css-stringify/issues/40.)
      • Relative to options.to.
      • Ignored and set to opitons.to if map.annotation is 'inline', or if map.annotation is 'preserve' and the previous annotation had the previous source map inlined.
    • map.setSourceContents:
      • If omitted, default to false.
      • false → Don’ set the sourceContents property of the source map. (Like now.)
      • true → Set the sourceContents property to the input CSS. (New feature, as requested by Inline source text along with map? #31.)
    • Perhaps: map.sourceRoot:
      • If omitted, don’t set the sourceRoot property of the source map. (Like now.)
      • Otherwise, set the sourceRoot property of the source map to map.sourceRoot. (New feature. It is very easy to add (new mozilla.SourceMapGenerator({sourceRoot: options.map.sourceRoot})), but YAGNI?)
    • Perhaps map.previous:
      • Intended as a replacement for map: sassMap, for example. However, does anyone use this? YAGNI?
      • If omitted, default to null (or possibly true).
      • null → Simply no previous source map.
      • Perhaps: false → Disable previous source map detection, but still generate a source map. If so: true → Autodetect previous source map (Like now.) YAGNI?
      • If an object:
        • previous.map:
          • If omitted, set options.previous to its default value (null or possibly true; see above).
          • It is a previous map, like map: sassMap now.
        • previous.path:
          • If omitted, default to '.'.
          • It is the path to the previous map. It is needed to correctly rewrite the source paths of the previous map to be relative to options.map.path.
        • Otherwise: previous.map = {map: previous.map}. (Like map: sassMap now.)
        • Or perhaps we should simply let map.previous be a previous source map (nothing more) and don’t care if it is not perfect.
• Deprecate options.mapAnnotation. Until it is removed:
  • false → options.map.annotation = 'none' (unless options.map.annotation was already set).
  • true → options.map.annotation = 'preserve' (unless options.map.annotation was already set).
• Deprecate options.inlineMap. Until it is removed:
  • false → Nothing to do.
  • true → options.map.annotation = 'inline' (unless options.map.annotation was already set).
• Deprecate trying to autodetect previous map by looking for options.from + '.map'. I don’t think that case exists in reality. The user could possibly pass from: from, map: {previous: {map: fs.readFileSync(from + '.map'), path: from + '.map'}} instead.

What do you think? I could make a PR.

[ai]

Yeap, I think about it too. You options changes is great and I agree with it. We can use it in 0.4 version and remove deprecated syntax in 0.5.

Also I think we should show deprecated warning on old options usage.

[ai]

I think map.previous should be true by default. Because a lot of users don’t understand source map, but they will be happy if magic will works.

So default value of map whould be { }. To generates map only if previous map was detected.

[ai]

Is map.setSourceContents too long? Maybe map.sourceContent?

[ai]

BTW, if we plan to change map API. Maybe we should add Result#mapObject? Sometimes we want to change genereated map, but it is not cool, that we need to parse string and recreate object.

[ai]

BTW. I have also 2 features for 0.4 release:

• better source map support if we have several input CSS;
• set sourceContent in map

[lydell]

Also I think we should show deprecated warning on old options usage.

Definitely.

I think map.previous should be true by default. Because a lot of users don’t understand source map, but they will be happy if magic will works.

The “magic” should be on by default, yes. The question was whether it is ever useful to be able to turn it off.

So default value of map whould be { }. To generates map only if previous map was detected.

That’s what I meant :)

Is map.setSourceContents too long? Maybe map.sourceContent?

Agreed.

BTW, if we plan to change map API. Maybe we should add Result#mapObject? Sometimes we want to change genereated map, but it is not cool, that we need to parse string and recreate object.

Could you expand a bit on this? I don’t really follow here ...

BTW. I have also 2 features for 0.4 release:

Do you mean that you have started working on them, or just plan them?

better source map support if we have several input CSS;

I didn’t know you could have several CSS inputs! Please tell me more about this. We need to consider it for the API changes.

How can you have several CSS inputs? Are they concatenated into one AST, and that AST is passed to the processing functions?

set sourceContent in map

That would be the options.map.sourceContents = true setting.

[ai]

Could you expand a bit on this? I don’t really follow here ...

Now Result object contains only stringified map in map property. It is difficult to change this generated map, because you need to parse JSON and next to create SourceMapGenerator instance. Maybe we should provide SourceMapGenerator of our map in Result?

Do you mean that you have started working on them, or just plan them?

Only planed :).

[ai]

About several inputs.

It is very important, because we need to support CSS concatination and some other magic stuff.

Right now users can get several inputs:

root1 = postcss.parse(css1, from: 'a.css')
root2 = postcss.parse(css2, from: 'b.css')

root1.rules = root1.rules.concat( root2.rules )

result = root1.toResult(to: 'app.css', map: true)

But it will disable source map autodetection, becase there was only one from option in toResult call. We need to write origin file to each CSS nodes.

[lydell]

Now Result object contains only stringified map in map property. It is difficult to change this generated map, because you need to parse JSON and next to create SourceMapGenerator instance. Maybe we should provide SourceMapGenerator of our map in Result?

Providing a SourceMapGenerator would be useful:

• You can avoid the mozilla.SourceMapGenerator.fromSourceMap(new mozilla.SourceMapConsumer(result.map)) dance.
• You can do result.map.toJSON() and result.map.toSting() to get an object or string representation.

I’m not sure how wise it is to expose another tool’s API, though.

If we don’t provide it, I’d vote for returning the map as an object, not as a string.

But it will disable source map autodetection, becase there was only one from option in toResult call. We need to write origin file to each CSS nodes.

Correct. I need to think about this.

[ai]

I agree, that we should return SourceMapGenerator in Result#map. Old API will still work, because fs.writeFile(result.map) will call toString() on result.map.

[ai]

I started a multiinputs branch.

OK, or you already start it?

[ai]

Done. I finished maps support with multiple inputs and pushed new source map code to v1.0 branch.

Maybe tomorrow I try to add object in Result#map and setSourceContent features.

[lydell]

Sorry, I’ve got side-tracked by lots of other projects, and I haven’t had any time to look at this :(

[ai]

@lydell don’t worry, I finish your options project :)

[ai]

@lydell why we need annotation: 'preserve'. Other options use preserve logic when you just miss value (like if you miss sourceContent PostCSS will look into previous maps).

[ai]

Deprecate trying to autodetect previous map by looking for options.from + '.map'.

Why you want this? For example, a some Grunt plugins generates maps file and PostCSS’s Grunt plugin developer don’t want to understand source maps and set correct options.

[ai]

BTW, today I also finish sourceContent feature.

[lydell]

@lydell why we need annotation: 'preserve'. Other options use preserve logic when you just miss value (like if you miss sourceContent PostCSS will look into previous maps).

I guess we don’t need 'preserve', then. Your idea is better.

[lydell]

Deprecate trying to autodetect previous map by looking for options.from + '.map'.

Why you want this? For example, a some Grunt plugins generates maps file and PostCSS’s Grunt plugin developer don’t want to understand source maps and set correct options.

I thought that it wasn’t used in reality, because all tools I’ve seen put a sourceMappingURL comment in the output. And I also thought about that CoffeeScript names the source maps like this: foo.coffee → foo.js + foo.map. But if you know of tools that justify the feature, then keep it :)

[ai]

Hm. About autodetect maybe you are right. There is no tool, that miss annotation,

[ai]

@lydell maybe we should generate sourcesContent by default?

[ai]

I refactored current options: 3b7d731

I will add new options on this week and we can close issue.

[ai]

Autodetection without annotation comment in CSS was disabled 9a6c042

[ai]

I decide to not implement sourceRoot option right now. It require to change all relative paths in map generator and I didn’t know user cases, which require special sourceRoot. Anyway we can add it later,

[lydell]

Yeah, if there is no use case I see no point in adding it. Just for the record, it wouldn’t be difficult to implement at all: Just do new SourceMapGenerator({sourceRoot: options.map.sourceRoot}) and then the source map module will take care of everything.

[lydell]

Don’t you want to support writing the source map to a different location than the same directory as the output CSS?

[ai]

@lydell and SourceMapGenerator will convert all our relative paths to new root?

[ai]

@lydell I already added annotation: 'another/path.map'

[lydell]

@lydell and SourceMapGenerator will convert all our relative paths to new root?

Actually, I’m not so sure anymore … let’s just leave it :)

@lydell I already added annotation: 'another/path.map'

Aha, I missed that. But it seems like that is unfinished? Shouldn’t the intended map path be used in MapGenerator::relative for example? Are there no tests for this feature?

[ai]

@lydell yeap, you right, I forget about MapGenerator#relative :(. I will fix it tomorrow.

[ai]

@lydell I fix relative paths 245daa7

[lydell]

What is the purpose of being able to pass the previous source map using options.map.prev?

I’d say it is to be able to do something like this:

file = "./source/style.ext"
{css, map} = preprocessor.compile(fs.readFileSync(file), source: file)
result = postcss(foo).process(css, from: file, map: {prev: map})

In other words, to be able to pass an in-memory map—a map that has never been written to disk.

I do not think the purpose of the options.map.prev option is to pass a map that has been written to disk. postcss finds such source maps automatically.

Why is this important?

• An example in the readme uses map: { prev: fs.readFileSync('from.sass.css.map') }, which I think it shouldn’t. It should be something more like my example.
• The same example in the readme can make new users think that they have to pass the previous map like this, when they actually just could let postcss do it for them automatically.
• Since options.map.prev is supposed to be an “in-memory” map, we can assume that all relative source paths in that map are relative to the current working directory. This is not the case when a map is read from a file: Then all relative source paths are relative to the map itself. There is (currently) no way of passing the path that relative sources are relative to. I suggested it in my initial comment of this issue, but I think that we shouldn’t do that since it complicates the options for no gain — provided that the options.map.prev is supposed to be used and not used as I described. Is it?
• If your previous is written to a file, then you should let postcss take care of it automatically for you. Then the path that relative sources are relative to can be preserved. However, it currently isn’t (see below).

PreviousMap::loadMap should set @sourcesRelativeTo:

• If prev → "."
• If @inline → path.dirname(@file)
• If @annotation → path.dirname(map)

Then in MapGenerator::applyPrevMaps the third parameter of .applySourceMap should be @relative(file.sourcesRelativeTo).

I wanted to make a PR, but three things stopped me:

• I wanted to make sure that I understand the options.map.prev option correctly, and that you agree.
• I’m unsure on where/how to write tests for it.
• Time.

So why did I write all of this?

Well, that’s because postcss’ previous source map support is incomplete in one place related to automatically finding the previous source map written to disk. I tried to fix it myself, but got stuck, because:

• The fix requires that my assumptions above are correct.
• I wasn’t sure on how to write the tests.

[ai]

@lydell you as usual make great analyis :).

You are totally right about prev map docs. Fixed: 70b26d6

Now we have same logic as you suggest:

1. If user set map.annotation → path.dirname(map)
2. If user set to → path.dirname(to)
3. If user set no options (but we require from and to option in docs for source map) → "."

[lydell]

Now we have same logic as you suggest:

But I can’t find the code …?

[ai]

@lydell maybe I am wrong and you are speaking not about MapGenerator#relative? https://github.com/ai/postcss/blob/v1.0/lib/map-generator.coffee#L110

[lydell]

Consider the following case:

• source/
  • source.css
  • source.sass
  • maps/
    • source.css.map
• build/
  • build.css
  • build.css.map

source.css.map contains "sources": ["../source.sass"].

source.css contains /*# sourceMappingURL=maps/source.css.map */.

We run this:

file = "source/source.css"
result = postcss(foo).process(fs.readFileSync(file).toString(), from: file, to: "build/build.css", map: true)
map = result.map.toJSON()

map (build.css.map) should then contain: "sources": ["../source/source.sass"], but it actually contains "sources": ["../source.sass"], because we didn’t tell .applySourceMap that source.css.map isn’t in the same directory as source.css. That’s why I suggested the @sourcesRelativeTo approach above.

[ai]

So problem is only we didn’t use map.annotation in .applySourceMap? OK, can you make PR please, because this relative paths blow up my minds :). I think I can make mistakes :).

[lydell]

The problem is the tests. Should the tests for this go into test/map.coffee? How do I build that directory structure? I see that you’re using fs-extra and stuff …

[ai]

@lydell test/map.coffee has @dir directory for fixtures. It cleans after every test.

[lydell]

How do I create files and directories into it?

[ai]

Just fs.writeFileSync()

[ai]

But, maybe you didn’t need to write anything. Just set map.prev.

[lydell]

And fs.mkdirSync()?

The whole point here is to not use map.prev! We’re testing the automatic previous source map finder.

[ai]

@lydell better use fs.outputFileSync() from fs-extra.

But, you don’t need to test map.prev finder. It already tested in test/previous-map.coffee. And sources path fixer depends only on map options, not on way, how you set prev map. So you can test sources fixes with map.prev.

[lydell]

The point is that, yes, you already test map.prev finder in test/previous-map.coffee and applying previous maps in test/map.coffee—but you never test the combination of those two, which is what this bug is about.

[ai]

OK, you are right. You can look at test/previous-map.coffee, how fixtures created there.

[lydell]

#55