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

PostCSS doesn't update SourceMaps properly when an input file is in a subdirectory #13

Closed
nDmitry opened this issue Dec 23, 2013 · 26 comments
Labels

Comments

@nDmitry
Copy link
Contributor

nDmitry commented Dec 23, 2013

├── b.css
├── b.css.map
├── in
│   ├── a.css
│   ├── a.css.map
│   └── a.sass
└── out
    ├── b.css
    └── b.css.map
$ cat in/a.css.map
{
"version": 3,
"mappings": "...",
"sources": ["a.sass"],
"file": "a.css"
}

$ autoprefixer -m in/a.css -o b.css    

$ cat b.css.map
{"version":3,"file":"b.css","sources":["in/a.css"],"names":[],"mappings":"..."}

$ autoprefixer -m in/a.css -o out/b.css

$ cat out/b.css.map                    
{"version":3,"file":"out/b.css","sources":["in/a.css"],"names":[],"mappings":"..."}

Expected output:

$ cat b.css.map
{"version":3,"file":"b.css","sources":["in/a.sass"],"names":[],"mappings":"..."}

$ cat out/b.css.map                    
{"version":3,"file":"out/b.css","sources":["../in/a.sass"],"names":[],"mappings":"..."}
@lydell
Copy link
Contributor

lydell commented Dec 23, 2013

I ran into the same issue when I developed my fork of Autoprefixer with source maps support. Here is how I solved the problem and the relevant test. If any of that helps ;) (I must admit that is not the most well-written code I've produced -.-)

I wonder if the source-map module could be smarter about this by default, though ...

@ai
Copy link
Member

ai commented Dec 23, 2013

@lydell thanks, it will be very helpful :).

@ai
Copy link
Member

ai commented Dec 23, 2013

Maybe we should commit to source-map? After few days I will start to fix this issue and I try to find how it will be difficult.

@lydell
Copy link
Contributor

lydell commented Dec 23, 2013

I think it has to do with that the .applySourcemap() method form the source-map module only works correctly if items of the sources array of source maps are exactly equal: If the in-sourcemap has sources: ["folder/foo.css"] and the out-sourcemap has sources: ["foo.css"] the applySourcemap() method does not understand that it's actually the same foo.css. So I used a lot of path.relative() and such to normalize the sources arrays first. Hopefully the source-map could do this, that would be the best for everyone of course.

@ai
Copy link
Member

ai commented Dec 23, 2013

There is problem, that path.relative() only in Node.js :(.

But we can use some smart autodetect and didn’t use this method in browsers.

@nDmitry
Copy link
Contributor Author

nDmitry commented Dec 23, 2013

@ai what about browserify shim?

@ai
Copy link
Member

ai commented Dec 23, 2013

You are cool guys :). Thanks, that is what I need.

Hopy, that mozilla team would like to add new dependencie :).

@nDmitry
Copy link
Contributor Author

nDmitry commented Dec 26, 2013

Проблему с обновлением mappings в grunt-autoprefixer обошел переходом в папку from, чтобы input (from) путь состоял только из имени файла — https://github.com/nDmitry/grunt-autoprefixer/blob/master/tasks/autoprefixer.js#L66:L69

Короче, в переводе на консольный интерфейс из этого: autoprefixer -m in/a.css -o out/b.css я сделал это: cd in && autoprefixer -m a.css -o ../out/b.css.

В этом случае mappings при имеющейся входящей карте обновляются корректно, хотя в file и sources тогда пути всё равно неправильные, поэтому их тоже пришлось указывать вручную: https://github.com/nDmitry/grunt-autoprefixer/blob/master/tasks/autoprefixer.js#L47:L55

@lydell
Copy link
Contributor

lydell commented Jan 24, 2014

Perhaps the second argument of .applySourcemap() should be used?

@lydell
Copy link
Contributor

lydell commented Jan 25, 2014

I tried changing the .applySourceMap() line to map.applySourceMap(prev, opts.from) and that partially solved the problem.

$ cat b.css.map 
{"version":3,"file":"b.css","sources":["a.sass"],"names":[],"mappings":"..."}

$ cat out/b.css.map 
{"version":3,"file":"out/b.css","sources":["a.sass"],"names":[],"mappings":"..."}

As you can see, the source is now changed to "a.sass", but the path to "a.sass" should be relative to the source map (as @nDmitry’s expected output shows).

This is how I understand the problem:

  1. Autoprefixer takes a source file as input. In this case, "in/a.css".
  2. Autoprefixer puts the source file in the sources array of the source map it creates: "sources": ["in/a.css"].
  3. Autoprefixer finds that "in/a.css" has a source map. It is a generated file, and therefore we don’t want to map to it, but to it’s source.
  4. Autoprefixer applies the source map for "in/a.css" ("in/a.css.map"), by using the .applySourcemap() method. That method works like this: "Hey, source map [the source map generated by Autoprefixer]. You contain a source that itself has a source map. For that source, map to the locations of that source map instead!". So the .applySourcemap() methods needs two things: The name of the source that itself has a source map (the second parameter), and the source map for that source (the first parameter). However, the method allows the second parameter to be omitted.
    • If the second parameter is omitted, the method tries to guess which source the source map to be applied belongs to, by looking at the file property. In my opinion, that’s not a good guess. The file property is optional, so it may be missing. Moreover, the spec just says that it is "a name of the generated code that this source map is associated with." It doesn’t say that it is the file name, or, more importantly, a path to the file. And as far as I know, browsers ignore this property.

      Still, the file property is used in this case. "in/a.css.map" contains "file": "a.css". So the method looks in Autoprefixer’s source map’s sources property for a file called "a.css". However, it only contains "in/a.css", so it cannot find anything. So, mission complete. There’s nothing for the method to do.

    • If the second parameter is not omitted, that parameter tells the method which source file the source map to be applied belongs to. If we send in "in/a.css" (opts.from) it works better. Now the method looks in Autoprefixer’s source map’s sources property for a file called "in/a.css". This time it find such a source. It rewrites the mappings for that source, and then replace "in/a.css" with "a.sass". However, it should have rewritten the path to "a.sass" to be relative to Autoprefixer’s source map, but unfortunately it didn’t. So I think this is a bug in the source-map module. When I think about it, it is not so strange. The .applySourcemap() hasn’t got all the information it needs. It also needs a path to the source map to be applied, relative to the master source map. In our case, it needs the path to "in/a.css.map" relative to Autoprefixer’s source map. In the first case: "in/a.css.map". In the second case: "../in/a.css.map".

So, here is what I think needs to be done:

  1. Postcss should provide the second parameter to the .applySourcemap() method (as I tested).
  2. The .applySourcemap() method should take a third parameter (as I described above). Moreover, I think that it should deprecate the use of just one or two parameters. Three should always be provided.
  3. Postcss should provide the third parameter to the .applySourcemap() method.

/cc @fitzgen

@ai
Copy link
Member

ai commented Jan 25, 2014

@lydell you are awesome :D thanks for good investigation.

@lydell
Copy link
Contributor

lydell commented Jan 25, 2014

Here is the test for .applySourceMap(). It does not cover sub-directories and the usage of the second parameter. That should be looked into as well.

@fitzgen
Copy link

fitzgen commented Jan 28, 2014

Assuming that there's tests, docs, and nice code, I'll accept a PR that adds the extra parameter to applySourceMap :)

@ai
Copy link
Member

ai commented Feb 10, 2014

@lydell you tell about take file from previus map (and of cource this idea is wrong, becuase it is option file), but why whe can’t change file option in previus map?

Can you please check my fix: a2e2652

It is seems to simple to me, maybe there are some side effects?

@ai
Copy link
Member

ai commented Feb 10, 2014

/cc @nDmitry @fitzgen

@ai ai added the 0.3 label Feb 11, 2014
@lydell
Copy link
Contributor

lydell commented Feb 11, 2014

That looks exactly like @map.applySourceMap(prev, @opts.from) to me. I'll look more at it asap hopefully tomorrow.

@ai
Copy link
Member

ai commented Feb 11, 2014

@lydell second argument for applySourceMap don’t solve all problems, because file and sources in generated map will not be relative from map file?

@lydell
Copy link
Contributor

lydell commented Feb 11, 2014

As far as I understand, your fix and using the second argument are equivalent. They both partially solve the problem: The .applySourceMap() method then correctly understand which source the source map to apply. belongs to. But it cannot know how to rewrite any sources from the source map to be applied relative to the new source map, so their paths will be wrong. (I could be wrong, though, because I couldn't test it when I wrote this).

Last Friday I started working on improving the .applySourceMap() method. However, I hadn't time to finish it and send a pull request. Hopefully I can do that tomorrow.

@ai
Copy link
Member

ai commented Feb 11, 2014

@lydell does I understand correct, that next problem is that we use apsolute path in map, but must use paths relative from map file?

I try to fix it in MapGenerator hack.

@lydell
Copy link
Contributor

lydell commented Feb 11, 2014

Source maps are free to use both absolute and relative paths. If they are relative, they should be relative to the source map itself. That's the problem. When we apply the previous map, the sources paths of that map will be put in our new map. But they will still be relative to the previous map. That's a bug in the .applySourceMap() method. Which I'm working on. No hack should be needed.

@nDmitry
Copy link
Contributor Author

nDmitry commented Feb 11, 2014

@ai just do what I did in grunt-autoprefixer or wait for the applySourceMap() fix.

  1. AFAIK, file property should be a name of a generated file, not a path. Check this out. Mozilla's lib doesn't work either without the property or if it's incorrect, so we should provide it if there isn't one in an input map. I use my function ensureFile() twice—on an input map to ensure that the property exists and it is correct, and once more when I get an output map (because source-map puts there a path).
  2. As lydell said, sources is a mess. I just take paths from an input map and make them relative to the output dir.

Also I don't want to look at all that CoffeeScript (sorry, it makes me sick =)), but make sure that your tests actually test mappings.

@ai
Copy link
Member

ai commented Feb 11, 2014

@nDmitry don’t be so cruel with CoffeeScript ;). There are a lot of execulent software on Ruby and Python with same code style :).

@lydell I think to take now path hacks from grunt-autoprefixer and clean them when source-map will be fixed right. Users too wait for 1.1 release and now I prefer hotfix :).

@lydell
Copy link
Contributor

lydell commented Feb 11, 2014

Sounds sensible 👍

@ai
Copy link
Member

ai commented Feb 12, 2014

@nDmitry @lydell I think I have done with relative paths :). Can you please check last 2 tests.

@lydell
Copy link
Contributor

lydell commented Feb 13, 2014

Looks good!

Good idea to hack fix this for the time being. I did get time to work on my patch yesterday, but I ran into several unexpected problems.

@ai ai closed this as completed Feb 13, 2014
@lydell
Copy link
Contributor

lydell commented Feb 13, 2014

mozilla/source-map#93

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants