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

Source Maps #125

Closed
shannonmoeller opened this issue Aug 8, 2013 · 156 comments
Closed

Source Maps #125

shannonmoeller opened this issue Aug 8, 2013 · 156 comments
Assignees
Milestone

Comments

@shannonmoeller
Copy link

Would be awesome to have sourceMap support.

https://developers.google.com/chrome-developer-tools/docs/css-preprocessors#toc-css-preprocessor-support

@GoalSmashers
Copy link
Contributor

@shannonmoeller please correct me if I'm wrong but source maps are for CSS preprocessors only. And apparently clean-css makes smaller CSS from… larger CSS!

@shannonmoeller
Copy link
Author

Source maps are for any JS or CSS files that are compiled from other source files. Because clean-css inlines @imports and modifies code to make it smaller than the source files, it qualifies! Consider your JS sister, uglifyjs. It only makes smaller JS files from larger JS files, and it supports generating source maps too!

@GoalSmashers
Copy link
Contributor

Ah, sneaky @imports! Challenge accepted :-)

@ghost ghost assigned GoalSmashers Aug 9, 2013
@Kienz
Copy link

Kienz commented Oct 22, 2013

+1
Any news?

@GoalSmashers
Copy link
Contributor

@Kienz we have some other tasks in the pipeline but we won't leave it unanswered. The more +1's the quicker we get into it.

@Subash
Copy link

Subash commented Nov 14, 2013

+1 This will be one of the most useful feature.

@iki
Copy link

iki commented Nov 15, 2013

+1, we'd prefer to use --clean-css from grunt less task, as it gives better results than --compress, but we need the source maps

@GoalSmashers
Copy link
Contributor

@iki @sbspk looks like we need to push it higher on the priorities list. Could be 2.1 then.

@iki
Copy link

iki commented Nov 16, 2013

@GoalSmashers sounds great, count me in for testing if needed

@Subash
Copy link

Subash commented Nov 16, 2013

@GoalSmashers sounds awesome 👍 .

@GoalSmashers
Copy link
Contributor

@iki @sbspk thanks, will do!

@lukeapage
Copy link
Contributor

When doing it please could you consider allowing passing in a source sourcemap as well? Uglifjs also allows this and I think If you use mozillas sourcemap library (on node) then it allows integrating it. I implemented css sourcemap support for less and it would be great to allow less to use clean css to compress the css and transform the sourcemap too.

@Subash
Copy link

Subash commented Nov 17, 2013

@lukeapage You stole the words out of my mouth :D +1 for the suggestion.

@GoalSmashers
Copy link
Contributor

@lukeapage @sbspk Sure, I'll take a look into it!

@joshuaspence
Copy link

+1

@cpixl
Copy link

cpixl commented Dec 30, 2013

+1
Really nice feature! :)

@ScottSmith95
Copy link

Looking forward to this an willing to help test out in any way I can.

@paazmaya
Copy link

paazmaya commented Jan 1, 2014

👍

@OliverJAsh
Copy link

I'm not sure how this can be done without an AST (and clean-css doesn't create one). Any thoughts @GoalSmashers?

LESS does create a source map, so it might be worth looking at what they're doing for compressing.

@GoalSmashers
Copy link
Contributor

@OliverJAsh I've been thinking a lot about it and we may need an AST-like structure to add source maps but there could be another option as well. Need to play a bit more with it to see what fits the best.

@dypsilon
Copy link

dypsilon commented Jan 9, 2014

+1

1 similar comment
@jberube
Copy link

jberube commented Jan 21, 2014

+1

@victormejia
Copy link

+1 This feature would be sooooo useful!

@gbmoretti
Copy link

+1

1 similar comment
@octavioamu
Copy link

+1

@frodeknutsen
Copy link

+1

@Tudmotu
Copy link

Tudmotu commented Dec 4, 2014

+1 :)

jakubpawlowicz added a commit that referenced this issue Dec 7, 2014
* Per discussion with Luke Page (see #125) it is used in case of shortened variables
  so probably useless when it comes to CSS.
@jakubpawlowicz
Copy link
Collaborator

Good news: I've tested it thoroughly during the last 2 days on my biggest project and it seems to works fine with the latest tweaks.

I tested 2 cases - no input source maps, just plain source CSS => minified CSS, and LESS => CSS => minified CSS, and in both cases it shows up fine in Chrome and Firefox.

The remaining two pieces is to check Windows + IE11 (they have source maps, right?) and write some docs on how to use it. Expect a merge to master soon!

@jakubpawlowicz
Copy link
Collaborator

@lukeapage I also created #397 for adding support for styles embedded in input source map.

@naartjie
Copy link

naartjie commented Dec 8, 2014

Happy days, this is awesome. I'm glad I found this repo, and this thread ;-) Thanks for work on source maps. 👍

I am trying to get source maps working in broccoli-clean-css plugin.

This line makes source get a value of __stdin__.css. Is there an option I can pass in to tell it where the original source file lives relative to the source map? I saw the question about __stdin__.css earlier, but if it was answered I missed it / it went over my head.

jakubpawlowicz added a commit that referenced this issue Dec 8, 2014
* Per discussion with Luke Page (see #125) it is used in case of shortened variables
  so probably useless when it comes to CSS.
@jakubpawlowicz
Copy link
Collaborator

@naartjie Sure thing! 💯

Regarding your question, that line applies when you provide input without a reference to a file. On the other hand if you pass a file, then clean-css will get all source map info from it.

We use the following trick to handle multiple input files correctly in CLI but I'm thinking about adding it to API too.

See: https://github.com/jakubpawlowicz/clean-css/blob/master/bin/cleancss#L107

@naartjie
Copy link

naartjie commented Dec 8, 2014

@jakubpawlowicz Nice magic trick with the @import ;-) Thank you kindly sir

This code gives an absolute path in the sources element. Is there a way to make that it a relative, without the leading /?

    var cleanCss = new CleanCss({sourceMap: true, root: rootDir});
    var mini = cleanCss.minify('@import url('+ relativeFilePath +');');

I tried setting relativeTo but guessing that is only for URL's inside the actual stylesheets?

This is working nicely for plain css files. I haven't tried with files which have source maps already, that'll be the next step, use it on files produced by a sass precompiler.

@jakubpawlowicz
Copy link
Collaborator

@naartjie Sure, just use target option pointing to a directory, e.g.

var cleanCss = new CleanCss({sourceMap: true, target: rootDir});
var mini = cleanCss.minify('@import url('+ relativeFilePath +');');

Here's some more info: https://github.com/jakubpawlowicz/clean-css#how-to-rebase-relative-image-urls

@jakubpawlowicz
Copy link
Collaborator

I've been able to confirm it works fine with Safari 7, but I have no idea if it does in Explorer 11.
It definitely supports source maps for JavaScript but I can't see any references to CSS nor be able to make it work. Any CSS maps seem to be silently ignored.

That won't block merging it into master, though.

@jakubpawlowicz
Copy link
Collaborator

@naartjie Btw, it'd be pretty awesome if you could give SASS + clean-css a try.

@jakubpawlowicz
Copy link
Collaborator

I consider the initial version of source maps as ready for 3.0 unless someone finds any bug(s). 🍻

@naartjie
Copy link

naartjie commented Dec 8, 2014

give SASS + clean-css a try

@jakubpawlowicz sure, I'm trying to get to it, I just ran into some problems this eve: #563 #565

@naartjie
Copy link

naartjie commented Dec 8, 2014

use target option pointing to a directory

When I changed root: srcDir to target: srcDir, I lost the whole source map, i.e. just an empty skeleton gets generated.

@gerbenvandijk
Copy link

🍺 awesome! hope it ends up in the grunt-contrib-cssmin repo soon as well 👍

@jakubpawlowicz
Copy link
Collaborator

@naartjie would you be able to share your code? I can take a look later today.
@gerbenvandijk it would be great indeed!

@jakubpawlowicz
Copy link
Collaborator

I've been able to verify that SASS source maps work fine too. I'll merge the branch to master now so please open a new issue if you spot anything wrong about source maps.

@naartjie
Copy link

I can also verify from my side, that SASS source maps in are working. Sorry it took a while.

👍 for merging it into master. Super cool.


Edit: just to be clear on this, I had to use version 3.0 of libsass for this to work, as it wasn't working on more recent libsass versions.

@jakubpawlowicz
Copy link
Collaborator

Great, thanks @naartjie for the feedback!

@naartjie
Copy link

@naartjie would you be able to share your code? I can take a look later today.

Sure thing. Here is a self contained test, which should exemplify my understanding of it:

var fs = require('fs');
var CleanCss = require('clean-css');

fs.writeFileSync('./my.css', 'body {color:red;}');

var cleanCss = new CleanCss({ sourceMap: true, target: process.cwd() });
var out = cleanCss.minify('@import url(my.css)').sourceMap.toString();

// out = {"version":3,"sources":[],"names":[],"mappings":""}

I'm sure I'm just doing something silly, or misunderstanding your instructions somehow. Thanks for looking into it.

@jakubpawlowicz
Copy link
Collaborator

That's how you should do it, however you're missing a semicolon in import: '@import url(my.css);'.

@naartjie
Copy link

missing a semicolon

Doh! that fixes it. Told you it was silly!! ;-) Thanks a mil!!

@jakubpawlowicz
Copy link
Collaborator

:-) Cheers!

@naartjie
Copy link

Cheers!

rhymes with... 🍻

@jakubpawlowicz
Copy link
Collaborator

I'll sure have once 3.0 is out :-D

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