Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate source mapping #2122

Closed
lydell opened this issue Jul 13, 2016 · 3 comments
Closed

Duplicate source mapping #2122

lydell opened this issue Jul 13, 2016 · 3 comments

Comments

@lydell
Copy link

lydell commented Jul 13, 2016

I followed this guide to set sassc up: https://github.com/sass/sassc/blob/master/docs/building/unix-instructions.md

Commits:

Steps to reproduce:

# Compile test file:
$ cat test.scss
a {
  k: v;
  b {
    k: v;
  }
}
$ ./bin/sassc -m test.scss test.css

# Inspect source map:
$ cat test.css.map
{
    "version": 3,
    "file": "test.css",
    "sources": [
        "test.scss"
    ],
    "mappings": "AAAA,AAAA,CAAC,CAAC;EACA,CAAC,EAAE,CAAE,GAIN;EALD,AAEE,CAFD,CAEC,CAAC,CAAC;IACA,CAAC,EAAE,CAAE,GACN",
    "names": []
}
$ npm install source-map
$ cat test.js
var fs = require('fs')
var sourceMap = require('source-map')
var map = fs.readFileSync(process.argv[2]).toString()
new sourceMap.SourceMapConsumer(map).eachMapping(function (m) {
  console.log(m)
})
$ node test.js test.css.map
# see below

Click here for full output of node test.js test.css.map

{ source: 'test.scss',
  generatedLine: 1,
  generatedColumn: 0,
  originalLine: 1,
  originalColumn: 0,
  name: null }
{ source: 'test.scss',
  generatedLine: 1,
  generatedColumn: 0,
  originalLine: 1,
  originalColumn: 0,
  name: null }
{ source: 'test.scss',
  generatedLine: 1,
  generatedColumn: 1,
  originalLine: 1,
  originalColumn: 1,
  name: null }
{ source: 'test.scss',
  generatedLine: 1,
  generatedColumn: 2,
  originalLine: 1,
  originalColumn: 2,
  name: null }
{ source: 'test.scss',
  generatedLine: 2,
  generatedColumn: 2,
  originalLine: 2,
  originalColumn: 2,
  name: null }
{ source: 'test.scss',
  generatedLine: 2,
  generatedColumn: 3,
  originalLine: 2,
  originalColumn: 3,
  name: null }
{ source: 'test.scss',
  generatedLine: 2,
  generatedColumn: 5,
  originalLine: 2,
  originalColumn: 5,
  name: null }
{ source: 'test.scss',
  generatedLine: 2,
  generatedColumn: 6,
  originalLine: 2,
  originalColumn: 7,
  name: null }
{ source: 'test.scss',
  generatedLine: 2,
  generatedColumn: 9,
  originalLine: 6,
  originalColumn: 1,
  name: null }
{ source: 'test.scss',
  generatedLine: 3,
  generatedColumn: 2,
  originalLine: 1,
  originalColumn: 0,
  name: null }
{ source: 'test.scss',
  generatedLine: 3,
  generatedColumn: 2,
  originalLine: 3,
  originalColumn: 2,
  name: null }
{ source: 'test.scss',
  generatedLine: 3,
  generatedColumn: 3,
  originalLine: 1,
  originalColumn: 1,
  name: null }
{ source: 'test.scss',
  generatedLine: 3,
  generatedColumn: 4,
  originalLine: 3,
  originalColumn: 2,
  name: null }
{ source: 'test.scss',
  generatedLine: 3,
  generatedColumn: 5,
  originalLine: 3,
  originalColumn: 3,
  name: null }
{ source: 'test.scss',
  generatedLine: 3,
  generatedColumn: 6,
  originalLine: 3,
  originalColumn: 4,
  name: null }
{ source: 'test.scss',
  generatedLine: 4,
  generatedColumn: 4,
  originalLine: 4,
  originalColumn: 4,
  name: null }
{ source: 'test.scss',
  generatedLine: 4,
  generatedColumn: 5,
  originalLine: 4,
  originalColumn: 5,
  name: null }
{ source: 'test.scss',
  generatedLine: 4,
  generatedColumn: 7,
  originalLine: 4,
  originalColumn: 7,
  name: null }
{ source: 'test.scss',
  generatedLine: 4,
  generatedColumn: 8,
  originalLine: 4,
  originalColumn: 9,
  name: null }
{ source: 'test.scss',
  generatedLine: 4,
  generatedColumn: 11,
  originalLine: 5,
  originalColumn: 3,
  name: null }

Here is the interesting part of the output:

{ source: 'test.scss',
  generatedLine: 3,
  generatedColumn: 2,
  originalLine: 1,
  originalColumn: 0,
  name: null }
{ source: 'test.scss',
  generatedLine: 3,
  generatedColumn: 2,
  originalLine: 3,
  originalColumn: 2,
  name: null }

That’s a duplicate mapping. This means that any program that consumes this source map and wants to go from line 3 column 2 doesn’t know if the answer is line 1 column 0 or line 3 column 2. Unfortunately, at least some tools choose the former, which ultimately results in the browser pointing to an unexpected location (3:2 → 3:2 is what a user expects.)

Real world trouble caused by this:

@mgreter
Copy link
Contributor

mgreter commented Jul 13, 2016

I don't really agree that it is invalid for libsass to output multiple source mappings for the same token, as such a token could be passed through multiple functions. Furthermore the "duplicate" you see is another "feature" we've added in response to #1747 (if you mean the ones before a b {).

The draft for SourceMap V3 unfortunately leaves a lot open in this regard ...

@lydell
Copy link
Author

lydell commented Jul 14, 2016

Thanks for your response! I think this is the situation:

  1. In the output a b { you map the a to a { in the original input (line 1) and the b to b { in the original input (line 3), because it is technically correct. I agree.
  2. To help browser dev tools, you added a second mapping for the a to b { in the original input (line 3). That's why there's a "duplicate" mapping.
  3. The .getOriginalPositionFor method of the mozilla/source-map library does not handle "duplicate" mappings. It takes a (line, column) pair as input and gives one as output, while it should really return an array of (line, column) pairs.
  4. PostCSS seems to add a single source mapping per selector. When merging a PostCSS source map and a Libsass source map it would help if PostCSS had made a higher-resolution source map, just like Libsass does.

So I guess there are several issues here:

  • PostCSS could generate better source mappings for selectors.
  • mozilla/source-map could start handling "duplicate" mappings.
  • Browsers could improve their source map consumption.
  • The spec could be clarified.
  • Libsass could possibly decide to optimize for what happens to work in practice, not what is correct (which would be a somewhat sad way to go.)

Feel free to close this issue if you like.

@mgreter
Copy link
Contributor

mgreter commented Jul 19, 2016

Agree with your conclusions. Not much libsass can do for now.
Thanks for your time to research and debug this.

@mgreter mgreter closed this as completed Jul 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants