Binding: Refurbishes binding to use new API #543

am11 · 2014-11-16T23:33:36Z

Libsass is getting a new API. It doesn't replaces the current one, but renders it somewhat obsolete.

Thanks to @mgreter; the new API is beauty! 👍

I have managed to implement it as drop-in replacement to the existing one, by revamping the bindings and context-wrapper.

Status:

Update binding.gyp to acquire new code files.
Update context-wrapper to act like an interface as well as helper to our bindings: now compilation takes place from context-wrapper.
Update binding.cpp to abide by the new API docs.
Make use of new JSON serialized errors (fixes Serialize errors as JS objects (optional?) #464 and CLI ouput in JSON OR any custom format libsass#235).
Make sure the code is cleaned up, no memory leaks, remove unused header inclusions and keep code readable.
Add variants of source-maps.
Parse source-map as JSON.
Make sure all existing tests are passing.
Once @mgreter's repo is merged and libsass' tagged release is available rebase and pull libsass and sass-spec submodules.

The new API also fixed: sass/libsass#634 and the corresponding node-sass test is updated and is passing.

am11 · 2014-11-16T23:41:06Z

@mgreter, can you please review the code and let me know if you find memory leaks or any other issue which needs to be addressed?

Out of 622, 619 tests are passing and the following three are failing:

api .renderFile(options) should save source paths relative to the source map file (link).
api .render({stats: {}}) should report correct source map in stats (link).
api .renderSync({stats: {}}) should report correct source map in stats (link).

Please let me know if there is something we need to update in order to pass those tests. I couldn't find a pattern. There are other passing tests with similar configuration. My first hunch was that they are failing due to the reason bar.scss file is empty, but even after adding some CSS code lines in it, these tests are failing.

am11 · 2014-11-16T23:41:41Z

src/sass_context_wrapper.cpp

+  void free_file_context(struct Sass_File_Context* fctx) {
+   // delete[] fctx->input_path;
+   // delete[] fctx->options.include_paths;
+   // delete[] fctx->options.image_path;


@mgreter, should we remove these?

They should be safe to remove, since sass_free_xxx_context should take care of that!

mgreter · 2014-11-17T17:29:18Z

src/binding.cpp

@@ -51,56 +32,63 @@ void ExtractOptions(Local<Value> optionsValue, void* cptr, sass_context_wrapper*
  }

  if (isFile) {


You can probably merge most or all of the following statements, as they now share a common struct (Sass_Options) to setup the options (before you had to split it up, since the options were living on two totally unrelated structs).

am11 · 2014-11-17T18:37:37Z

Thanks for the review @mgreter. I have made those changes in 4abc7dd. :)

mgreter · 2014-11-17T18:37:41Z

Please see commit mgreter@f375c37 for a fix for the failing unit test.
IMO the bindings could be simplified even more!
Let me see if I can come up with something in an hour or two :)
P.s. it should be documented better that the command to run the test suite is:
./node_modules/.bin/mocha

am11 · 2014-11-17T18:42:45Z

Great! I will try it now.

For running tests, you can use npm test in node-sass root directory.

I follow this sequence:

# (1)
cd into/node-sass/dir

# (2) first get latest libsass
cd src/libsass
git pull --rebase mgreter-libsass api/c-interface
cd ../..

# (3) now install npm, only if you haven't already
#     just once (a while; in case we modify dependencies or newer version of dependencies
#     have arrived)
npm install  # optionally you can delete npm_modules before running this command,
             #  so you don't have unused / outdated dependencies laying around

# (4) now rebuild so it doesn't use the old binaries
node scripts/build -f

# (5) finally run the test
npm test
node scripts/build -f

Repeat (4) and (5) if you are making changes in cpp code. :)

@mgreter

This code is ported from @mgreter's fork.

am11 · 2014-11-17T18:54:37Z

Now all tests are passing. 😄

Thanks again @mgreter. 👍

mgreter · 2014-11-17T19:05:20Z

OK, glanced over the code and refactored some bits and removed some obsolete parts. Test still pass, but I would strongly recommend to do at least some basic memory leak testing. In perl-libsass I just use a lot of the spec tests in an endless loop to see if memory stabilizes at some point.
mgreter@7bc9479

am11 · 2014-11-17T19:23:19Z

Thanks can you please rebase your branch so I can pull your changes.

Also, for memory leaks, wouldn't it make sense to use preexisting tools like http://valgrind.org/downloads/current.html#current to detect the memory leaks? I haven't tried it yet. :)

mgreter · 2014-11-17T19:38:21Z

Sure, Valgrind would be much better if you know how to use them properly with node.js ;)
But a poor mans memory leak test should still show the most obvious memory leaks!

am11 · 2014-11-17T19:50:27Z

LOL, first I would have to compile it on Windows, and if that works then there is a learning curve for its usage. :)
So poor man's memory leak test that is. 😄
Or may be something like this: http://stackoverflow.com/a/2981185/863980.
Now I am realizing that for memory leaks, I am like that one-trick pony who just learnt how to detect it by monitoring task-manager or dry running the code. 👎

mgreter · 2014-11-17T19:59:35Z

Sorry, rebase has to many conflicts. Maybe you can scavenge some bits from the diffs in my branch.

am11 · 2014-11-17T20:01:24Z

Yup I will give it a try now. BTW, with latest sass-spec master, we are passing all 626 tests with your api/c-interface !

@mgreter

Credit goes to @mgreter.

am11 · 2014-11-17T20:44:37Z

@andrew, with this API, we can get error messages in both formats; JSON and (the old) plain text. I changed it to get JSON. Do we need to make it optional as well? I don't think with code usage anyone would not prefer JS-object. TBH, on CLI it also looks nicer. For instance, in CLI:

C:\users\Adeel\Source\Repos\node-sass [master]> node bin/node-sass --stdout c:/temp/foo3.scss
{
  "status": 1,
  "file": "c:/temp/foo3.scss",
  "line": 3,
  "column": 9,
  "message": "invalid syntax"
}

Secondly; do we need to parse the errors as object? So in onerror: callback, the consumer will just do error.message, error.column , error.line and error.status. For other errors generated within our JS code (like fs I/O ops), we can wrap them in object {message: err}.

Thoughts?

andrew · 2014-11-17T22:44:23Z

I don't see why not, when we release this I expect we'll bump it to 2.0.0 so breaking changes are to be expected.

am11 · 2014-11-17T22:49:00Z

Awesome! 👍
Hopefully, we will get source-maps inlining and other varieties from libsass as well. We would need to exposes options for those (and someday, sooner or later, we would be consuming other charming features like; custom functions and sass-importer).

For CLI format as JSON with indent = 2.

am11 · 2014-11-18T00:35:52Z

@andrew, see 9f7a78d; on CLI it will also print JSON errors. :)

Also fixes the issue with destination path being resolved w.r.t input, without accounting for it being already absolute.

am11 · 2014-11-18T02:08:50Z

With d58c303, I have added two new source-map options:

sourceMapEmbed: embed sourceMappingUrl as data URI.
sourceMapContents: embed include contents in maps.

Addresses #372.

^ This needs test cases. 😊

latest libsass, fixing x64 problem

Fixes sass#546.

iamvdo · 2014-12-11T11:34:41Z

When will this PR be integrated? Any though?

am11 · 2014-12-11T12:22:41Z

Will it depends on libsass' next tagged release. This is related: sass/libsass#697.

Binding: Refurbishes binding to use new API

andrew · 2014-12-18T11:23:11Z

am11 · 2014-12-18T11:33:09Z

Yay! 🎉

mgreter · 2014-12-18T11:36:31Z

👍

leehambley · 2014-12-21T18:29:44Z

Forgive me posting here not being a Node.js'er, but looks like this didn't make it into the real release yet? I noticed that node-sass doesn't make use of milestones (neither do I), and that the last release was about ~2.5 months ago, can we rely on a note here when the new version including the commit goes live? I can barely wait to abandon Ruby sass for my front end build!

Exports quote and unquote functions for C bindings

Binding: Refurbishes binding to use new API.

af72ccc

am11 reviewed Nov 16, 2014
View reviewed changes

Code: Cleanup.

739cc08

am11 mentioned this pull request Nov 17, 2014

Named colours don't work as map keys sass/libsass#652

Closed

mgreter reviewed Nov 17, 2014
View reviewed changes

Code: Mitigates chances of memory leaks.

4abc7dd

Binding: Fixes for stats.

9e436db

This code is ported from @mgreter's fork.

Code: Brings more optimizations.

a8c6710

Credit goes to @mgreter.

Errors: Convert errors to JS objects.

9f7a78d

For CLI format as JSON with indent = 2.

Options: Adds options for source-maps varieties.

d58c303

Also fixes the issue with destination path being resolved w.r.t input, without accounting for it being already absolute.

am11 force-pushed the master branch from 3fc82df to d58c303 Compare November 18, 2014 02:03

am11 mentioned this pull request Nov 18, 2014

Source maps for sass_context sass/libsass#650

Closed

This was referenced Nov 18, 2014

inline source map #372

Closed

Provide api for custom importers #530

Closed

am11 referenced this pull request in sass/libsass-net Nov 18, 2014

bump versions

7f7abb3

latest libsass, fixing x64 problem

am11 mentioned this pull request Nov 19, 2014

Consider adding support for sourcesContent to the source map #363

Closed

am11 added the v2 label Nov 19, 2014

Source-map: Parse as JSON.

67ea9c5

am11 mentioned this pull request Nov 19, 2014

omitSourceMapUrl not working? #546

Closed

am11 force-pushed the master branch from d4fa18b to b7d868d Compare November 19, 2014 22:50

Code: Fixes condition is-output-path-relative.

187cfde

Fixes sass#546.

am11 force-pushed the master branch from b7d868d to 187cfde Compare November 19, 2014 22:53

am11 mentioned this pull request Nov 25, 2014

calling render with null data results in 'malloc pointer being freed was not allocated' #554

Closed

am11 mentioned this pull request Dec 15, 2014

Improved documentation for CLI #572

Closed

am11 changed the title ~~Binding: Refurbishes binding to use new API [DO NOT MERGE]~~ Binding: Refurbishes binding to use new API Dec 18, 2014

am11 added a commit that referenced this pull request Dec 18, 2014

Merge pull request #543 from am11/master

7c354bf

Binding: Refurbishes binding to use new API

am11 merged commit 7c354bf into sass:master Dec 18, 2014

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024

Merge pull request sass#543 from mgreter/expose-quote-function

f660ede

Exports quote and unquote functions for C bindings

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binding: Refurbishes binding to use new API #543

Binding: Refurbishes binding to use new API #543

am11 commented Nov 16, 2014

am11 commented Nov 16, 2014

am11 Nov 16, 2014

mgreter Nov 17, 2014

mgreter Nov 17, 2014

am11 commented Nov 17, 2014

mgreter commented Nov 17, 2014

am11 commented Nov 17, 2014

am11 commented Nov 17, 2014

mgreter commented Nov 17, 2014

am11 commented Nov 17, 2014

mgreter commented Nov 17, 2014

am11 commented Nov 17, 2014

mgreter commented Nov 17, 2014

am11 commented Nov 17, 2014

am11 commented Nov 17, 2014

andrew commented Nov 17, 2014

am11 commented Nov 17, 2014

am11 commented Nov 18, 2014

am11 commented Nov 18, 2014

iamvdo commented Dec 11, 2014

am11 commented Dec 11, 2014

andrew commented Dec 18, 2014

am11 commented Dec 18, 2014

mgreter commented Dec 18, 2014

leehambley commented Dec 21, 2014

		@@ -51,56 +32,63 @@ void ExtractOptions(Local<Value> optionsValue, void* cptr, sass_context_wrapper*
		}

		if (isFile) {

Binding: Refurbishes binding to use new API #543

Binding: Refurbishes binding to use new API #543

Conversation

am11 commented Nov 16, 2014

am11 commented Nov 16, 2014

am11 Nov 16, 2014

Choose a reason for hiding this comment

mgreter Nov 17, 2014

Choose a reason for hiding this comment

mgreter Nov 17, 2014

Choose a reason for hiding this comment

am11 commented Nov 17, 2014

mgreter commented Nov 17, 2014

am11 commented Nov 17, 2014

am11 commented Nov 17, 2014

mgreter commented Nov 17, 2014

am11 commented Nov 17, 2014

mgreter commented Nov 17, 2014

am11 commented Nov 17, 2014

mgreter commented Nov 17, 2014

am11 commented Nov 17, 2014

am11 commented Nov 17, 2014

andrew commented Nov 17, 2014

am11 commented Nov 17, 2014

am11 commented Nov 18, 2014

am11 commented Nov 18, 2014

iamvdo commented Dec 11, 2014

am11 commented Dec 11, 2014

andrew commented Dec 18, 2014

am11 commented Dec 18, 2014

mgreter commented Dec 18, 2014

leehambley commented Dec 21, 2014