Add support for embedding source files into the source map

[simonexmachina]

Using sourcesContent property as per the source maps spec. This PR resolves #212.

Be aware that I'm not a C developer, so I'm really making guesses here although I have enlisted the help of someone who does know C. I think it's correct, but I don't have the knowledge to write tests or really validate that what I've done is correct.

I've added integration for this into node-sass but all I get is a segfault :( Happy to pursue it further if you have some suggestions but I feel that it might be better for someone with some clues about C to take it from here :)

FYI, the JSON code is taken from CCAN.

[mgreter]

Hadn't had a close look at the sources, but the travis build only seems to break because you didn't add the source files for the json lib to the automake build system (makefile.am). Maybe you could give us more info when it segfaults (how to reproduce).

[simonexmachina]

I've added the source files and Travis is happy. There's people asking for this, any chance we can move this forward?

[akhleung]

This looks like a significant addition, so I'll need a bit more time to review. Thanks!

[simonexmachina]

Sure @akhleung, no problem. Look forward to hearing from you.

[am11]

On a similar note; please take a look at #324.

The gist is:

• With LESS compiler, the level of detail captured in source-map is very very extensive: It captures sm-info for each property-name, value and even part of value if required (in case you are using vars, mixins etc.).
• With ruby-sass, the level of detail is (correct but) somewhat mediocre: upto the declarations. Really not as good as LESS. IMO, it looks bit experimental, not the finalized implementation.
• With libsass, the level of detail is wooly. At some points, it is even incorrect and erroneous! And the maximum valid information it reflects; the line number of rule. Not even the correct column number is captured.

I know this is not usually what happens around here, but please (only this time -- for once) consider taking LESS' lead, because they really did a great job making source-maps versatile, most comprehensive and hence quite reliable. I believe sass gem (ruby version) will also be fixed at some point to capture more / all tokens language info (which is the main point of having source-map feature in first place, in source-to-source trans-compilations).

With LESS set as criteria, you can make corresponding sample code in LESS for comparing source-maps and fix libsass' mappings until the results are ditto. :)

That will complete source-map feature in libsass, once forever!

Thoughts?

[HamptonMakes]

We, really really need a test here. I'd LOVE to merge this in, but I don't think I should till I can be sure it breaks less than the last version. I'm not really familiar with this code at all.

[Ventajou]

I wish I could help, this would really help improve our setup at work.

[simonexmachina]

I'd love to help here but I've got a few things that prevent me from doing so:

1. I know zero about C++ and wouldn't have any idea how to write tests for this
2. I've changed my mind about how to handle this, and this is no longer a solution that I'd use
3. I won't be able to work on this until after November

These are pretty simple changes, is there anybody else who can put together some tests so this can be merged in?

[HamptonMakes]

Hmmm... just noticed the Copyright notice and license on the Json files. I might contact the person whose email is indicated and ask if we can remove the notice, put it under our license (MIT), and give him credit in the Readme?
I'm a little wacky about this kind of stuff, since we want to make sure that the code is solidly able to be owned by a Sass foundation in the future. We have a lot of businesses involved in the Sass community now, so I get a little paranoid on this kind of stuff. Like to keep it streamlined!

[HamptonMakes]

Oh, apparently he's on github! Hey @joeyadams, mind if we remove the copyright notice from the file and add a shout out to you in the Readme. This is an MIT-licensed project.

[HamptonMakes]

Alright, I'm going to email @joeyadams and hope that we can get this sorted out!

[joeyadams]

Hooray, I'm useful! Yes, you can remove the copyright notices from the files and put them in the readme.

I took a quick look at the code, and noticed a few issues in SourceMap::generate_source_map:

• It looks like it's wrapping the sourcesContent array in two pairs of brackets (json_stringify will generate array brackets when passed an array).
• This is a memory leak: result += json_stringify(triples, "\t");. json_stringify returns a char*, which is not freed automatically like a C++ string.
• This function could be rewritten to use the JSON library instead of string concatenation to build the whole object.

[simonexmachina]

Thanks @joeyadams, I've removed the extra square brackets. How can I resolve the memory leak? Forgive my ignorance but I know basically zero about C :)

[HamptonMakes]

@joeyadams WOO! Thanks!
@aexmachina Looks like this needs to be rebased now.

[joeyadams]

@aexmachina: to fix the memory leak, change this:

result += json_stringify(triples, "\t");

to this:

char *str = json_stringify(triples, "\t");
result += str;
free(str);

Also, I'd change the name triples to something more descriptive, maybe sourceContent.

[simonexmachina]

Thanks @joeyadams, I've made those changes. @hcatlin I've rebased now as well. Pretty sure the merge is correct, but I suggest this needs tests and I'm not the person to write them.

[am11]

@aexmachina, this is awesome! I think one of the reasons why the build is failing is because resolve_relative_path returns string, while resolve_and_load returns char*.

@mgreter, would it make sense to change resolve_relative_path to return char* as well; to make the function signatures consistent?

file.hpp#L13 and file.cpp#L120 to:

char* resolve_relative_path(const string& uri, const string& base, const string& cwd)

file.cpp#L171 to:

return result.c_str();

If not then @aexmachina, you would need to change the calls to resolve_relative_path( .. ) in context.cpp to resolve_relative_path( .. ).c_str().

[am11]

@mgreter, @xzyfer,

⌚

Guys should we merge it in and fix it? OP did his best. This is big step forward to have JSON support. IMO, we should serialize errors/exception messages in JSON rather than noisy/verbose XML (or at least make it optional, so the compatibility with ruby-sass doesn't get effected -- I don't know if that is needed for error messages too). See sass/node-sass#464.

[xzyfer]

I don't think we should merge something that's failing master. Having master fail impedes progress on other features.

Also from other sourcemap related discussion it seems that few of us (myself included) really understand sourcemaps to make the call here. I would defer to @akhleung and @hcatlin on this one.

[mgreter]

This has to be fixed as it IMO introduces some memory leaks. I also don't like that it stores the complete source twice (there is also a sources vector). Will see what I can do:
-> https://github.com/mgreter/libsass/tree/source-map-sources

[simonexmachina]

Thanks @mgreter, really glad that someone who knows C++ is looking at this :)

[mgreter]

Closing this in favor of #591.