Add support for sourcemaps #28

webuniverseio · 2016-11-15T15:55:31Z

No description provided.

nitriques · 2016-11-16T01:21:39Z

Hey @szarouski, this is great! But can you fix the build ? Right now, it's not working. See https://travis-ci.org/t32k/grunt-csso/jobs/176102824. You can run it locally before pushing, npm run test. Thanks!

webuniverseio · 2016-11-18T21:55:21Z

Fixed. Don't know how I missed travis 😛

webuniverseio · 2016-11-27T18:33:35Z

@nitriques does it look good now?

nitriques · 2016-12-14T21:12:43Z

tasks/csso.js

-        }).css;
+        });
+        css = result.css;
+        map = result.map || map;


The || map is useless IMHO

nitriques · 2016-12-14T21:13:11Z

tasks/csso.js

      }
      catch (err) {
        return next(err);
      }

-      if (proceed.length === 0) {
+      if (css.length === 0) {


it would be better to do !css

nitriques · 2016-12-14T21:14:24Z

Yes! Sorry @szarouski I was a lot busy in the past weeks. I've made another quick review: could you check it/argue with me/fix it ? Thanks!

webuniverseio · 2016-12-23T01:16:31Z

@nitriques Thanks for review! 🍺 Updated.

nitriques · 2017-01-06T21:08:06Z

tasks/csso.js

+          }
+          if (map) {
+              grunt.file.write(mapDest, map);
+              grunt.log.write('File ' + chalk.cyan(mapDest) + ' created' + (options.report ? ': ' : '.'));


One last thing ! Both writes could happen in parallel with fs.writeFile. If you do not want to do it, just tell me ;)

@szarouski ping!

@nitriques Hey, I didn't forget, just didn't have time yet. I'll look into that during weekend :)

Ok great ! Ping me if you have any questions.
If you can't do it, I'll merge as is and do it ;) Thanks again!!!

webuniverseio · 2017-01-14T03:43:51Z

test/csso-test.js

+      fs.readFileSync('tmp/sourcemap.css', 'utf8'),
+    'should create sourcemap');
+    test.equal(
+      fs.readFileSync('test/expected/sourcemap.css.map', 'utf8').replace(/(\\r)?\\n/g, eol === '\r\n' ? '\\r\\n' : '\\n'),


Had to write those replacements due to the way how sourcemaps are generated. Hacky but tests pass on windows, so i hope it is not a big deal.

Yeah I'll try to create a better solution ;) Not a problem right now ;) Maybe force EOL to LF in tests...

nitriques · 2017-01-15T22:11:36Z

@szarouski Thanks! Check out the attached commits I've just pushed ;) And please test on Windows !

Grunt offers us a way to set globally the EOL characters so we should use it. Simply usign the OS version is not ideal since many Windows users would like to have LF endings. Re #28

Which is faster then writing a file Re #28

async.map will do exactly what we need here, i.e. wait for 1 or 2 operations to complete. Report any errors. Re #28

* Add support for sourcemaps 7b968bc * Add support for sourcemaps 22598a9

Grunt offers us a way to set globally the EOL characters so we should use it. Simply usign the OS version is not ideal since many Windows users would like to have LF endings. Re #28

Which is faster then writing a file Re #28

async.map will do exactly what we need here, i.e. wait for 1 or 2 operations to complete. Report any errors. Re #28

webuniverseio · 2017-01-16T00:53:05Z

@nitriques, checked out latest version from dev tests are failing on windows for sourcemap:

Running "nodeunit:tests" (nodeunit) task
Testing csso-test.js.FF......
>> csso - sourcemap
>> Message: should create sourcemap
>> Error: '.a{-webkit-fi1ter:blur(5px);filter:blur(5px)}.b{-webkit-padding-before:10px;pad
ding-block-start:10px}\r\n/*# sourceMappingURL=sourcemap.css.map */' == '.a{-webkit-fi1ter
:blur(5px);filter:blur(5px)}.b{-webkit-padding-before:10px;padding-block-start:10px}\n/*#
sourceMappingURL=sourcemap.css.map */'
>>   at Object.sourcemap (test\csso-test.js:20:10)
>>   at runCallback (timers.js:574:20)
>>   at tryOnImmediate (timers.js:554:5)
>>   at processImmediate [as _immediateCallback] (timers.js:533:5)

>> csso - sourcemap
>> Message: should create sourcemap
>> Error: '{"version":3,"sources":["<unknown>"],"names":[],"mappings":"AAAA,E,CACC,wB,CACA
,gB,CAED,E,CACC,2B,CACA,wB","file":"<unknown>","sourcesContent":[".a {\\n\\t-webkit-fi1ter
: blur(5px);\\n\\tfilter: blur(5px);\\n}\\n.b {\\n\\t-webkit-padding-before: 10px;\\n\\tpa
dding-block-start: 10px;\\n}\\n\\n/*# sourceMappingURL=data:application/json;base64,eyJ2ZX
JzaW9uIjozLCJzb3VyY2VzIjpbInNvdXJjZS5jc3MiXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IkFBQUE7SUFDSSww
QkFBa0I7WUFBbEIsa0JBQWtCO0NBQ3JCO0FBQ0Q7SUFDSSw2QkFBMEI7WUFBMUIsMEJBQTBCO0NBQzdCIiwiZmlsZS
I6InNvdXJjZS5jc3MiLCJzb3VyY2VzQ29udGVudCI6WyIuYSB7XG4gICAgZmlsdGVyOiBibHVyKDVweCk7XG59XG4u
YiB7XG4gICAgcGFkZGluZy1ibG9jay1zdGFydDogMTBweDtcbn1cbiJdfQ== */"]}' == '{"version":3,"sour
ces":["<unknown>"],"names":[],"mappings":"AAAA,E,CACC,wB,CACA,gB,CAED,E,CACC,2B,CACA,wB","
file":"<unknown>","sourcesContent":[".a {\\r\\n\\t-webkit-fi1ter: blur(5px);\\r\\n\\tfilte
r: blur(5px);\\r\\n}\\r\\n.b {\\r\\n\\t-webkit-padding-before: 10px;\\r\\n\\tpadding-block
-start: 10px;\\r\\n}\\r\\n\\r\\n/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJz
aW9uIjozLCJzb3VyY2VzIjpbInNvdXJjZS5jc3MiXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IkFBQUE7SUFDSSwwQk
FBa0I7WUFBbEIsa0JBQWtCO0NBQ3JCO0FBQ0Q7SUFDSSw2QkFBMEI7WUFBMUIsMEJBQTBCO0NBQzdCIiwiZmlsZSI6
InNvdXJjZS5jc3MiLCJzb3VyY2VzQ29udGVudCI6WyIuYSB7XG4gICAgZmlsdGVyOiBibHVyKDVweCk7XG59XG4uYi
B7XG4gICAgcGFkZGluZy1ibG9jay1zdGFydDogMTBweDtcbn1cbiJdfQ== */"]}'
>>   at Object.sourcemap (test\csso-test.js:24:10)
>>   at runCallback (timers.js:574:20)
>>   at tryOnImmediate (timers.js:554:5)
>>   at processImmediate [as _immediateCallback] (timers.js:533:5)

>> csso - comments
>> Message: should minify and remove all but the first exclamation comment
>> Error: '.a{-webkit-fi1ter:blur(5px);filter:blur(5px)}.b{-webkit-padding-before:10px;pad
ding-block-start:10px}\r\n/*# sourceMappingURL=sourcemap.css.map */' == '.a{-webkit-fi1ter
:blur(5px);filter:blur(5px)}.b{-webkit-padding-before:10px;padding-block-start:10px}\n/*#
sourceMappingURL=sourcemap.css.map */'
>>   at Object.comments (test\csso-test.js:34:10)
>>   at runCallback (timers.js:574:20)
>>   at tryOnImmediate (timers.js:554:5)
>>   at processImmediate [as _immediateCallback] (timers.js:533:5)

Warning: 3/12 assertions failed (15ms) Use --force to continue.

nitriques · 2017-01-16T15:16:05Z

Ok thanks. At home, I do not have a Windows machine (I've tested in on Ubuntu) but I do at work. Will test ASAP.

nitriques · 2017-01-16T20:14:01Z

I have no problems running the test on my Windows machine.

I think it depends on your git config: if git converts LF to CRLF, then the test might not pass.

I've juste pushed a .gitattributes file to make sure your git config does not make the test fails.

webuniverseio · 2017-01-16T22:52:55Z

I had an issue even after I pulled, so I decided to delete everything except for .git and try again. After reset everything works fine. Good job 👍

Add support for sourcemaps

7b968bc

webuniverseio force-pushed the feature/sourcemaps-support branch from dc91a63 to 86d5c92 Compare November 18, 2016 21:53

nitriques changed the base branch from master to dev December 14, 2016 21:09

nitriques requested changes Dec 14, 2016

View reviewed changes

webuniverseio force-pushed the feature/sourcemaps-support branch from 86d5c92 to 8e4ad56 Compare December 23, 2016 01:14

nitriques requested changes Jan 6, 2017

View reviewed changes

webuniverseio force-pushed the feature/sourcemaps-support branch from 8e4ad56 to 671ebd6 Compare January 14, 2017 03:31

Add support for sourcemaps

22598a9

webuniverseio force-pushed the feature/sourcemaps-support branch from 671ebd6 to 22598a9 Compare January 14, 2017 03:42

webuniverseio commented Jan 14, 2017

View reviewed changes

nitriques approved these changes Jan 15, 2017

View reviewed changes

nitriques merged commit 2703de5 into t32k:dev Jan 15, 2017

nitriques pushed a commit that referenced this pull request Jan 15, 2017

Fix EOL issues

60abf09

Grunt offers us a way to set globally the EOL characters so we should use it. Simply usign the OS version is not ideal since many Windows users would like to have LF endings. Re #28

nitriques pushed a commit that referenced this pull request Jan 15, 2017

Use mkdir to create directories

4c2202c

Which is faster then writing a file Re #28

nitriques pushed a commit that referenced this pull request Jan 15, 2017

Use async module to handle fs.writeFile calls

d9eb921

async.map will do exactly what we need here, i.e. wait for 1 or 2 operations to complete. Report any errors. Re #28

nitriques pushed a commit that referenced this pull request Jan 15, 2017

Add support for sourcemaps (#28)

86cdd1a

* Add support for sourcemaps 7b968bc * Add support for sourcemaps 22598a9

nitriques pushed a commit that referenced this pull request Jan 15, 2017

Fix EOL issues

7eda915

Grunt offers us a way to set globally the EOL characters so we should use it. Simply usign the OS version is not ideal since many Windows users would like to have LF endings. Re #28

nitriques pushed a commit that referenced this pull request Jan 15, 2017

Use mkdir to create directories

d46d061

Which is faster then writing a file Re #28

nitriques pushed a commit that referenced this pull request Jan 15, 2017

Use async module to handle fs.writeFile calls

2d02cf0

async.map will do exactly what we need here, i.e. wait for 1 or 2 operations to complete. Report any errors. Re #28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sourcemaps #28

Add support for sourcemaps #28

webuniverseio commented Nov 15, 2016

nitriques commented Nov 16, 2016

webuniverseio commented Nov 18, 2016

webuniverseio commented Nov 27, 2016

nitriques Dec 14, 2016

nitriques Dec 14, 2016

nitriques commented Dec 14, 2016

webuniverseio commented Dec 23, 2016

nitriques Jan 6, 2017

nitriques Jan 12, 2017

webuniverseio Jan 12, 2017

nitriques Jan 13, 2017

webuniverseio Jan 14, 2017

nitriques Jan 15, 2017

nitriques commented Jan 15, 2017

webuniverseio commented Jan 16, 2017

nitriques commented Jan 16, 2017

nitriques commented Jan 16, 2017

webuniverseio commented Jan 16, 2017

Add support for sourcemaps #28

Add support for sourcemaps #28

Conversation

webuniverseio commented Nov 15, 2016

nitriques commented Nov 16, 2016

webuniverseio commented Nov 18, 2016

webuniverseio commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitriques commented Dec 14, 2016

webuniverseio commented Dec 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitriques commented Jan 15, 2017

webuniverseio commented Jan 16, 2017

nitriques commented Jan 16, 2017

nitriques commented Jan 16, 2017

webuniverseio commented Jan 16, 2017