Source Map support #109

Closed
ai opened this Issue Aug 11, 2013 · 47 comments

Comments

Projects
None yet
Contributor

ai commented Aug 11, 2013

It is very important, because a lot of users will use source maps from Sass/Stylus. And if Rework will destroy this map data, if will be bad feature :(.

Contributor

ai commented Aug 11, 2013

For example, Source Map support is blocking for some users in Autoprefixer: ai/autoprefixer#37

ai referenced this issue in postcss/autoprefixer Aug 11, 2013

Closed

Source maps #37

Grawl commented Aug 12, 2013

Agree. I need both source maps and autoprefixer in developing.

Source Map support would be excellent as without it, troubleshooting styles post-processed with autoprefixer becomes impossible in Chrome as it uses Source Maps for Sass = > CSS tracing.

+1 for this, for the same reason. I want to use Autoprefixer and retain working sourcemaps

+1. Need to keep my source maps if I'm using autoprefixer.

Owner

tj commented Aug 13, 2013

+1 from me, I don't know the implementation details of source maps but I imagine we can do it pretty easily as a third-party library, maybe with a few more details in the AST

Contributor

ai commented Aug 13, 2013

@visionmedia I can help, but we need some API ideas :).

Mozilla has good source-library. But it is very basic support, so wee need another layout.

It’s possible to create this source-map-under-rework library if css lib will save all spaces and symbols (like display/**/: inline.

So, it will be possible to has some lib like:

var map = require('rework-source-map');
var css  = rework('a { … }');

map.removeRule(1);
map.toString(); //=> CSS content
map.toMap();    //=> Source Map content
Contributor

ai commented Aug 13, 2013

@visionmedia so, for this library css lib must save all data to create original CSS from AST.

Rowno commented Aug 18, 2013

This should help debugging the source map: http://sokra.github.io/source-map-visualization/

andreypopp referenced this issue in reworkcss/css-stringify Nov 2, 2013

Closed

Initial support for source map generation #23

Contributor

lydell commented Nov 8, 2013

Here is what needs to be done:

  • Make sure that css-parse 1.6.0 or later is used.
  • Parse using both the position and source options of css-parse.
  • Make css-whitespace also emit position and source in the AST.
  • Possibly adjust a few rework plugins to be more careful with the position and source of the AST.
  • Make css-stringify able to emit source maps (using position and source of the AST) (see also sheet and my fork of it).
  • Add source map generation to the rework API.

So there is not much that needs to be done to rework itself. Most of the work will be on adding source map support to css-stringify.

tj was assigned Nov 8, 2013

Contributor

kristoferjoseph commented Nov 15, 2013

🍻

Contributor

andreypopp commented Nov 28, 2013

Source map support landed in css-stringify version 1.4.0 — now one could quite easily add it to rework.

@andreypopp And Styl hopefully ^_^

Contributor

andreypopp commented Nov 28, 2013

@yoshuawuyts for Styl it would be a little bit more difficult cause css-whitespace parser doesn't track the position in AST as far as I know, but I don't think it would be hard to add this feature.

Owner

tj commented Nov 28, 2013

we could add it to styl and at least get close to the original values haha, css-stringify will have that issue anyway with injecting things unless plugins are really careful about retaining position data

Owner

tj commented Nov 28, 2013

or maybe in stringify we should have ignore those (unless we do already) so it falls back on the previous, for example if you were to take a @Keyframes and clone it into @-webkit-keyframes and @-moz etc that's a big gap it creates but right now i dont think any plugins copy over the position props

Contributor

lydell commented Nov 28, 2013

Don't worry about source map support for the plugins—I'd guess that most of them just works. I added source map support to the CSS parser and stringifier for autoprefixer, and then the source maps were perfect—even though nothing in autoprefixer was built with source maps in mind! For example, autoprefixer can clone your @keyframes rules into @-webkit-keyframes etc, and those prefixed blocks were mapped back to the original unprefixed block, just like that. After all, we're just cloning js objects, which happen to contain the data needed for source maps (original line, column and file). Let's see what simply works and what needs a little tweaking.

What should the API be like? .toString({sourceMap: true}) // {code: "...", map: ...}?

Contributor

andreypopp commented Nov 28, 2013

@visionmedia maybe we can provide some API for plugins to work with position info?

Contributor

andreypopp commented Nov 28, 2013

@lydell yeah, you are right, most of them just work — rework-inherit, rework-macros, rework-vars, ... I use them with source maps with no issues.

Contributor

andreypopp commented Nov 28, 2013

@lydell changing return value based on argument isn't very good API. Yes, I confess, I did that in css-stringify, but if css-stringify is a "low-level" solution then rework is kinda "user facing product".

Maybe it would be a better idea just to inline source map as base64 encoded comment? For development it's very useful and easy to use. I don't really see use cases for using source maps in production... "debugging CSS" — is that a thing?

Contributor

lydell commented Nov 28, 2013

If you write a web app as free software, it is really nice to be able to view the source code just by opening the browser's dev tools.

Contributor

andreypopp commented Nov 28, 2013

@lydell inlined base64 encoded source maps will do that

Owner

tj commented Nov 28, 2013

like @lydell mentioned it's just a matter of cloning, I'm not a huge fan of ast node "classes" for an api, unnecessary complication in this case, anything higher level we need to optimize can just be little node modules like rework-visit

Contributor

kristoferjoseph commented Nov 29, 2013

@visionmedia I am ready and willing to implement this. Don't want to waste anyone's time though. Can you outline the API you want please?
Thanks!

Owner

tj commented Nov 29, 2013

i think we can maybe get away without it entirely, as long as css-stringify uses the position data that it last saw, it wont really matter that new stuff like https://github.com/visionmedia/rework/blob/master/lib/plugins/prefix-value.js#L47 wont have it

Contributor

kristoferjoseph commented Nov 29, 2013

You don't want to expose an API for plugin developers? I am cool with that, just wondering if this might open the door for silly bugs being filed later. Possibly all that is needed is an entry to the README explaining how to generate and use sourcemaps? An entry on how not to destroy position data? What do we think?

Contributor

lydell commented Nov 29, 2013

No special API for plugins is needed. @visionmedia's specific example could easily get source map support by just adding position: decl.position (didn't try it though). Simple as that. The only API issue is how rework users should tell rework that they want a source map and how they should get it.

Owner

tj commented Nov 29, 2013

well if we just use the last position data it should get us to the same spot without even assigning .position. for example if you clone @Keyframes into 3 other keyframes, just use that position by keeping a stack of positions in css-stringify for declarations/rules

Contributor

kristoferjoseph commented Nov 29, 2013

@lydell @visionmedia
How about:

rework(read(data, 'utf8'), {debug: true});
Owner

tj commented Nov 29, 2013

I think we're talking about two different things, as far as css-parse / rework go we shouldn't have to change anything

Contributor

kristoferjoseph commented Nov 29, 2013

@visionmedia Sorry, I must be dense. How will a rework user specify that they want sourcemaps generated? Will they be on by default?

Contributor

andreypopp commented Dec 3, 2013

@visionmedia @kristoferjoseph @lydell see #123 — I propose inlining source maps as base64 encoded data URIs — that would provide the best dev experience, I think.

Contributor

andreypopp commented Dec 3, 2013

@kristoferjoseph .toString(options) already takes options arg which it forwards to css-stringify so .toString({sourcemap: true}) would work after we bump css package version. But see my previous comment — I think inlining source maps is a better idea.

Contributor

kristoferjoseph commented Dec 3, 2013

@andreypopp @visionmedia @lydell that is exactly what I was looking for. How do we feel about inlining the source maps?

Contributor

lydell commented Dec 3, 2013

+1 to inlining the source maps. I love the simplicity of it.

Btw, @andreypopp you meant "see #123" above.

Contributor

andreypopp commented Dec 3, 2013

@lydell thanks, fixed

Owner

tj commented Dec 3, 2013

you'd want sourcemaps in production as well though, maybe an option to inline

Contributor

andreypopp commented Dec 3, 2013

Actually I don't see the use case for having sourcemaps in production. In case of JavaScript code you would want to debug exceptions but in case of CSS?

For those rare cases when you still want separate sourcemaps maybe it is better just to provide a command line utility which extracts it from the bundle — extract-source-map bundle.css > bundle.map. What do you think?

nwalton3 commented Dec 3, 2013

I may be mistaken, but I think chrome dev tools expects a separate source map file.

Contributor

andreypopp commented Dec 3, 2013

@nwalton3 nope, it works well with inlined source maps too — I use this technique with andreypopp/xcss.

mgol commented Dec 3, 2013

There is one practical limitation (though maybe not major) - sometimes you might want to open the resulting CSS file to ensure sth didn't get broken; having a huge source map blob at the end of the file can significantly slow the editor.

Contributor

andreypopp commented Dec 5, 2013

@mzgol hm... broken in what sense? If it's broken there should be a bug somewhere in rework's component so it should be files as an issue... does this happen frequently? I'm also not having problems with opening resulting CSS files with vim or less...

Contributor

andreypopp commented Dec 5, 2013

@visionmedia @lydell @kristoferjoseph any objections about related PR or can we merge it? Just don't want it to become stale :-)

mgol commented Dec 5, 2013

@andreypopp Jetbrains IDEs sometimes choke on files that have extremely long lines. I guess this could be mitigated by using a non-compresses source map representation (Lo-Dash uses sth like that, just do npm -g install lodash, try lodash modern -p and look at the map file).

mgol commented Dec 5, 2013

@andreypopp Sometimes the breakage may not be because of rework but e.g. because some watcher tool didn't invoke rework properly. The simplest way to check that is to just open the resulting CSS file.

Contributor

jonathanong commented Dec 23, 2013

closing this for now since. please open any new source map issues separately! hopefully after #6 and #89 are done, we can release v1!

Contributor

kristoferjoseph commented Dec 23, 2013

So excited!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment