gruntjs / grunt-contrib-stylus

Compile Stylus files to CSS.
http://gruntjs.com/
MIT License
174 stars 70 forks source link

Adds sourcemaps support #117

Closed rudolf closed 9 years ago

rudolf commented 10 years ago

Adds Stylus sourcemap support.

Because of https://github.com/LearnBoost/stylus/issues/1669 it requires specifying the dest option.

I can add documentation if you're happy with the implementation.

freezy commented 10 years ago

Correct me if I'm wrong, but isn't the sourceRoot parameter ignored in your patch? It would be handy though.

rudolf commented 10 years ago

No, it gets passed down to stylus which includes it in the resulting sourcemap file as "sourceRoot":"/"

rudolf commented 10 years ago

Did you expect it to act differently? It might be helpful to share your thoughts in https://github.com/LearnBoost/stylus/issues/1669 I personally found it much harder to get stylus to produce the correct sourcemaps than for instance coffeescript or uglifyjs.

freezy commented 10 years ago

Yeah, just noticed. I've actually expected the whole path to the map to be replaced, as of now it gets only prefixed to the current path. My styles are sitting at client/styles, while the URL to the compiled files is /css, so prefixing won't do any good.

vladikoff commented 10 years ago

This seems to fail with this config

sourcemap: {
        files: {
          'tmp/sourcemap.css': ['test/fixtures/stylus.styl', 'test/fixtures/stylus2.styl']
        },
        options: {
          paths: ['test/fixtures/include'],
          sourcemap: {
            sourceRoot: '/',
          },
          dest: 'tmp' // Grunt ignores this, but it's required to produce the 
                      // correct relative paths, see stylus issue #1669
        }
      },

Result is

{"version":3,"sources":["../test/fixtures/stylus.styl"],"names":[],"mappings":"AACA,KACE,KAAK,UACL,UAAU","file":"stylus.css","sourceRoot":"/"}
{"version":3,"sources":["../test/fixtures/stylus2.styl"],"names":[],"mappings":"AACA,QACE,KAAK","file":"stylus2.css","sourceRoot":"/"}

image

pixeldrew commented 9 years ago

Funnily enough, before seeing your PR I attempted the same thing and wrote it almost identically. The only thing i'd add, when using sourcemaps, 1:1 is the only compile method as you can only have one map comment per file. 8eb630195f5176bb662b393872f7d9217cffdf05

rudolf commented 9 years ago

@pixeldrew Thanks, that answers @vladikoff 's problem. I'll add that into this PR too.

In tasks/stylus.js:68 you test for if(options.sourcemap && options.sourcemap.comment) is there a reason you'd only write out the sourcemap if comment is true? I don't know why someone would want to set it to false, but when they do, they'd probably still expect a map file to be written to disk.

pixeldrew commented 9 years ago

The other option of sourcemap 'inline' puts the map out in the output and seems to be the only way to get sourcemaps to work correctly for now. The path added when using 'comment' seems to not play nice with grunt-contrib-stylus' output path. The only way it really works is if you have the styl and css files in the same directory.

AliMD commented 9 years ago

@skaapgif what's going on ?

flying-sheep commented 9 years ago

does concatenating the sourcemaps like this really work?

maybe that’s why inline is the only way that works right now…

pixeldrew commented 9 years ago

My version of the PR has a check to see if there is more than one input because you can't concat sourcemaps together into one file since they are a JSON structure. Also the generated files would have more than one /sourceMap=/ comment which wouldn't be honored by most editors.

The generated CSS files made by stylus have the sourcemap attribute added to the file with the original path of the stylus files. A regular expression search and replace on the last line of the file to replace the path made by Stylus with the location of the new grunt-stylus-contrib generated css/map would fix the problem.

I can add that to my PR

pixeldrew commented 9 years ago

This is done https://github.com/gruntjs/grunt-contrib-stylus/pull/121

rudolf commented 9 years ago

@pixeldrew seems to have more time on his hands than I do and since our implementations are so close I'm happy to continue the conversation in #121

sindresorhus commented 9 years ago

Closing in favor of #121