[WIP] Implements embedding source contents in source maps

[mgreter]

Implementation fallows source map spec (v3).

[edit] Added todo tasks for QA:

• use long flags in travis-ci config
• get (a) json library to compile with msvc2013
• pass all node-sass source map tests

This is based on PR #443 by @aexmachina

• Inclusion of source contents in source maps.
• Embeding sourceMappingUrl as data uri (Base64)
• Complete source map output generating with json library.

Also streamlined status information for included sources. Added helper function add_source to build the storage for the status information, collecting this data:

• queue: Internal queue of items to process
• included_files: Absolute path to the included file
• include_links: Relative path (to source_map_file)
• sources: The content of the include (is freed on Context destruction)
• source_map.source_index: Index to access the status info on the Context

IMO we could also store the content on each source_map, since it will anyway be just a pointer to the data stored in sources (that's how the original code by @aexmachina worked). But it was also handy to have direct access to all options and status information. I pass ctx to source_map.generate_source_map and it will use the index to access the information. I also made the status vectors private, so nobody beside the helper function can mess with it (therefore it should ensure that all of them have always the same size).

I also store the initial include on to these vectors, as it may provide usefull information if needed, and one can simply skip the first entry if only the "real" includes are interesting.

This probably need some discussion, as it adds two new options:

+ // embed include contents in maps
+ bool source_map_contents;
+ // embed sourceMappingUrl as data uri
+ bool source_map_embed;

The first option enables to inclusion of source contents inside the source map.
The second is used to embed the source map inside the sourceMappingUrl (via data-uri).

Maybe someone has a better idea how to name them. I've choosen source_map_contents because the sources will be added to a property named sourcesContent (was source_map_sources in org. PR).

For source_map_embed I had to add a Base64 encoding library.
@hcatlin: I couldn't find a good library with explicit MIT license, but IMO it should be ok to use:

Dedicator recognizes that, once placed in the public domain, the Work may
be freely reproduced, distributed, transmitted, used, modified, built upon, or
otherwise exploited by anyone for any purpose, commercial or non-commercial,
and in any way, including by methods that have not yet been invented or conceived.

// CC @xzyfer, @am11: do you think this is already good to go?

[mgreter]

Let's see if we can improve that code coverage score :)
Don't think that we want to include our external libraries!

[coveralls]

Coverage increased (+1.63%) when pulling bb91e27 on mgreter:source-map-sources into 5e86443 on sass:master.

[am11]

Its positive again! ✔️
👍

Should we have tests for it on SASSC?

[mgreter]

Is it possible to test this kind of stuff with sassc easily?
Not sure if there is anything setup to do it at the time beeing?
IMO most interface stuff is not yet tested directly by sassc/libsass!?

[coveralls]

Coverage increased (+1.62%) when pulling 1b00718 on mgreter:source-map-sources into 2fae47a on sass:master.

[HamptonMakes]

The license looks fine to me to remove the notice, since they are declaring
it public domain, we can put it under our MIT license.

On Wed, Oct 29, 2014 at 6:42 PM, Coveralls notifications@github.com wrote:

[image: Coverage Status] https://coveralls.io/builds/1402029

Coverage increased (+1.62%) when pulling 1b00718
1b00718
on mgreter:source-map-sources into 2fae47a
2fae47a
on sass:master.

Reply to this email directly or view it on GitHub
#591 (comment).

[xzyfer]

nitpick: it's good manners use long flags where possible. Make it easy to grok without consulting documentation.

[mgreter]

OK, I will have a look at how I can write this as a multiline command and will change it back to long flags again!

[xzyfer]

IIRC this might work

export FOO="foo --bar bar \
    --baz baz \
    --bam bam"

[xzyfer]

I'm not overly familiar with sourcemaps but there's nothing here that jumps out at me 👍

[am11]

@mgreter, I have pulled your branch and json.cpp presented new challenges when compiling with VS2013' cl.exe. snprintf is only present in VS2014, which is in preview right now. I was managed to grab the guard for it. Now it is giving me linker errors: context.obj : error LNK2001: unresolved external symbol _base64_encode_block.

Incidentally, there were three warnings json.cpp#L1175-L1177 and they are fixed by this orthodox way: http://stackoverflow.com/a/15486664/863980 😄

instead use:

*b++ = 0xEFu;
*b++ = 0xBFu;
*b++ = 0xBDu;

[xzyfer]

I wonder if it's possible for travis to run our builds against Windows so we can catch these issue sooner.

[am11]

Travis-CI doesn't have Windows. Appveyor is the one we use on node-sass. It has perl, python, VS2013, MinGW etc. Take a look at this: sass/libsass-net@94abbd7#commitcomment-8333599. :D

I tried but didn't get too far with that.. Only if we can get cl.exe compatible nmake file, then we can configure it.

[mgreter]

Is there a upstream repository for this json library on github?
We probably should raise also an issue there about windows compilation!?

[am11]

That does not seem to be available on GitHub. It's over here: https://git.ozlabs.org/?p=ccan;a=tree;f=ccan/json.

We can probably checkout @pb82's repository: https://github.com/pb82/JSON.cpp.

[mgreter]

I guess we got the main technical problems covered (which still need to be fixed), but what about the naming of the options?

+ // embed include contents in maps
+ bool source_map_contents;
+ // embed sourceMappingUrl as data uri
+ bool source_map_embed;

Any better ideas (since I'm not really convinced that these are the best)?

[coveralls]

Coverage increased (+1.63%) when pulling 53a568a on mgreter:source-map-sources into 7a381b8 on sass:master.

[mgreter]

@am11: IMO snprintf is only needed to create error messages. Maybe you can come up with something that will compile on windows (_snprintf ??). I guess we could also live with a solution that uses conditionals (#ifdef).

[am11]

_snprintf is different than snprintf. There is a slight chance that the upcoming update of VS2013 (update 4) would get the standard stdio.h's snprint back-ported from VS2014 community-technology-preview. Otherwise, we can retire this conditional #ifdef (guard code) when VS2014 is RTW'd.

Incidentally, should we look for other repos for json.cpp on GitHub as well? For instance, this one looks promising: https://github.com/open-source-parsers/jsoncpp.

[mgreter]

Honestly the current json lib feels much leaner than jsoncpp. And it's a pitty that only the error reporting is a problem and not the actual implementation ... but yeah, it may be the only viable solution. But IMHO the author of the current json lib is on github, so we may can get this that way working!? As always, feel free to make a PR ;) // CC @joeyadams

[am11]

Will do that! :)

[am11]

@mgreter, sent the PR to your repo: mgreter#1. 😎

[mgreter]

👍 Lets see what travis ci has to say :)

[mgreter]

It should now behave as before (at least for the node-sass tests). Will now create some unit tests of my own for perl-libsass and will then check it against before and after. @am11 as a side note, the watch test does not exit on my machine and leaves the node process running.

[mgreter]

Got it to pass all unit tests on my machine (Win7):
https://github.com/mgreter/node-sass/tree/fix/libsass-windows-issues

There were some other minor issues:

• Some expected files had different linefeeds than generated by libsass
• I had some weird unit test failed because of process.chdir(__dirname);:

  1) compile file should throw an exception for bad input:
     AssertionError: Missing expected exception..
      at Function._throws (assert.js:300:5)
      at Function.assert.throws (assert.js:317:11)
      at Context.<anonymous> (D:\temp\node-sass\test\test.js:189:17)
      at Test.Runnable.run (D:\temp\node-sass\node_modules\mocha\lib\runnable.js:217:15)
      at Runner.runTest (D:\temp\node-sass\node_modules\mocha\lib\runner.js:373:10)
      at D:\temp\node-sass\node_modules\mocha\lib\runner.js:451:12
      at next (D:\temp\node-sass\node_modules\mocha\lib\runner.js:298:14)
      at D:\temp\node-sass\node_modules\mocha\lib\runner.js:308:7
      at next (D:\temp\node-sass\node_modules\mocha\lib\runner.js:246:23)
      at Object._onImmediate (D:\temp\node-sass\node_modules\mocha\lib\runner.js:275:5)
      at processImmediate [as _immediateCallback] (timers.js:345:15)

  2) render to file should raise an error for bad input:
     Uncaught AssertionError: success() should not be called
      at sass.renderFile.success (D:\temp\node-sass\test\test.js:228:9)
      at D:\temp\node-sass\lib\index.js:288:16
      at Object.<anonymous> (D:\temp\node-sass\test\test.js:203:7)
      at Function.invoke (D:\temp\node-sass\node_modules\sinon\lib\sinon\spy.js:157:59)
      at Object.proxy (eval at createProxy (D:\temp\node-sass\node_modules\sinon\lib\sinon\spy.js:70:86), <anonymous>:1:39)
      at options.success (D:\temp\node-sass\lib\index.js:282:8)
      at options.success (D:\temp\node-sass\lib\index.js:165:7)

The watch process is still not terminating correctly!

[am11]

@mgreter, thanks for adding more tests for node-sass! Please move the css/scss files to test/fixtures/<suitable directory>/ (/cc @andrew, @kevva)

I will try running your tests now.

Meanwhile, FWIW -- I use PowerShell with the following approach:

(given you have Python 2.7.8 and VS2013 Express for Desktop 2013 installed)

cd ./into/node-sass/

# if you haven't already, run
npm install -g node-gyp

# 1- given you have the remote:  git remote add node-sass https://github.com/sass/node-sass
git pull --rebase node-sass master 

# clear out the old pacakges
rm -r node_modules

# 2- install npm will download the dependencies and rebuild libsass binary,
# it then clones into sass/node-sass-binaries repo and copies the
# prebuild binaries (of all OS'es architectures into ./bin/)
npm install

# 3- its a bug in our build script that npm install first build the binary, copies it to ./bin/
# then it overwrites that binary as part of copying binaries from node-sass-clone,
# therefore, remove the windows binaries:
rm -r build; rm -r ./bin/win32-ia32-v8-3.14 -Force; rm -r ./bin/win32-x64-v8-3.14 -Force

# 4- if you want to modify ./src/libsass, now is the time, then:
node ./lib/build.js

npm test
# or you can just do "node" and enter node.js interactive shell
# where you can use: var sass = require("./lib");
# to get a node-sass object.

If you want to modify ./src/libsass after running test, repeat step 3 and 4.

[am11]

I pulled your branch and tested with latest libsass, it passes all the tests and exited gracefully.

I had to change ./binding.gyp though:

diff --git a/binding.gyp b/binding.gyp
index 6b8e506..743888f 100644
--- a/binding.gyp
+++ b/binding.gyp
@@ -8,6 +8,7 @@
         'src/libsass/ast.cpp',
         'src/libsass/base64vlq.cpp',
         'src/libsass/bind.cpp',
+        'src/libsass/cencode.c',
         'src/libsass/constants.cpp',
         'src/libsass/context.cpp',
         'src/libsass/contextualize.cpp',
@@ -19,6 +20,7 @@
         'src/libsass/file.cpp',
         'src/libsass/functions.cpp',
         'src/libsass/inspect.cpp',
+        'src/libsass/json.cpp',
         'src/libsass/node.cpp',
         'src/libsass/output_compressed.cpp',
         'src/libsass/output_nested.cpp',

See my branch: https://github.com/am11/node-sass/tree/mgreter-changes.

[kevva]

Yeah, you def need to rebase to mirror the latest master before PR. A lot of changes happened in the tests structure.

[mgreter]

There are two commits. I just used the test.js from some other node-sass. I did not create those tests ;) Anyway, that's why I made two commits. The first one is only adding fixes, while the second adds the additional unit tests.

Anyway, probably the wrong thread to discuss this any further!
@am11 feel free to take over anything you can use to node-sass!

IMHO this PR is ready to get merged! // CC @am11, @xzyfer, @hcatlin

[am11]

@mgreter for this PR, should we consider mappings: issue: #601 (comment) separate? That regression belongs to this IMO.
Other than that, it's perfectly ready! :)

[mgreter]

I cleaned up the commits and also added a guard for the problem reported in #601.
I will merge this in soon if no other issues are reported in the meantime!