Custom functions

[matryo]

This is a refactor of #530 (custom importers) to also support #332 (custom functions).

The mechanism to synchronize the worker thread that runs libsass and the main v8 loop has been centralized and abstracted into an object called CallbackBridge that has two specializations: CustomImporterBridge and CustomFunctionBridge. The former supports the existing functionality (#530) while the other makes it possible to register custom functions that are backed up by v8 functions and takes care of translating Sass_Values to something that makes sense to the JS runtime and vice versa.

The function arguments are relayed to the JS callback by means of wrapping Sass_Value* pointers into v8 objects. Those objects mimic the API offered by libsass and as such could be a bit cumbersome to work with on the javascript side. This problem could however be largely mitigated by translating those wrapper objects into JS primitives upon entering the JS callback and back into wrapper objects upon return. The only edge cases that I can think of is that of SASS_NUMBER values when they carry units and SASS_COLOR where we could parse strings and detect patterns like \d+(px|em|%) and rgba?\(...)\ or resolve to use more JS friendly facades.

This is a quick example demonstrating how compass' headings function may be implemented:

sass.render({
  data: '#{headings(2,5)} { color: #08c; }',
  functions: {
    'headings($from: 0, $to: 6)': function(from, to) {
      var i, f = from.getValue(), t = to.getValue(),
          list = new sass.types.List(t - f + 1);

      for (i = f; i <= t; i++) {
        list.setValue(i - f, new sass.types.String('h' + i));
      }

      return list;
    }
  }
});

[coveralls]

Coverage increased (+0.63%) to 92.94% when pulling 3311b4f on matryo:custom_functions into 98f300e on sass:master.

[am11]

@matryo, first of all thanks for the effort. 👍

Welcome to GitHub! Wow you joined today and your first PR is HUGE! I would have split it into more commits so to make it easy to manage.

You are aiming to address #332, which is in our 2.1 milestone v3 milestone (actually there is no v2.1, we are taking a leap from v2.0.1 straight to v3.0). Therefore, we will consider this after v2.0.0 is shipped. I would have discussed it before going through this kind of major change.

Regarding the implementation, we decided to implement this format:

sass.render({
  data: '#{headings(2,5)} { color: #08c; }',
  functions: {   // the idea is to make these definitions least complicated

    headings: function (from, to) {
      return processIt(from, to);
    },

    anotherHeadings: function (from, to, done) {
      done(processIt(from, to));
    }

  }
});

which is achievable if you keep the custom functions definition in C++ keeping JS usage syntax flexible (the way we did for custom importer).

Thirdly, I am not sure about the new Sass types you added (new sass.types.String('h' + i))). Are these arbitrary types? In general we do not add additional syntaxes in node-sass beyond what libsass has to offer. Especially not to solely meet the requirement of a separate framework such as Compass, which is beyond the scope of this project.

@mgreter, @xzyfer, any thoughts on this?

[mgreter]

Looks pretty ok to me 👍 Also the new types are pretty much expected from my side, since they represent the values possbile in libsass (https://github.com/sass/libsass/wiki/API-Sass-Value). I personally also implemented/overloaded "toString" for each type, which could solve some "issues" @matryo mentioned when handling such types in node (maybe take a look at how I did it for perl-libsass).

I also like the config part by @matryo better, since the snippet @am11 posted is not taking advantage of the "possibilities" libsass offers here. There is no way to define default values for arguments, if you do not explicitly include the function signature somewhere. And I also do it that way in perl-libsass!

[matryo]

@am1 I understand that this PR's approach is pretty unusual and I am really sorry about that. I did not realize any one had already started a draft in that direction. Just to give you some context this was a major road block for us and I needed to get this completed in a couple of days, which unfortunately did not leave me much opportunity to consult the maintainers team. That said I am totally eager to take all the time necessary to make sure this code is in line with what you guys envisioned for 2.1 if you think this is a solid base for #332.

[am11]

@mgreter, now that you have described it, the syntax looks pretty OK to me too. 😄

Would it make sense to have both syntaxes for function descriptor? Mostly the function definitions lack default values. So, when we have this special need for default parameter:

'headings($from: 0, $to: 6)': function (from, to) {
  return processIt(from, to);
},

otherwise:

headings: function (from, to) {
  return processIt(from, to);
},

@matryo, we may need to refine it and this will take some time to digest the whole PR.

[am11]

CodeStyler: This is a sorted list.

[mgreter]

Btw. I find it pretty cool that @matryo was able to pull off this implementation without asking anybody about the internals directly! I hope that the time I've put into the libsass wiki was worth it! @matryo : If you have anything to add to the Wiki, please do so!!

@am11 you could handle functions without signatures as if they would be registered with fn(...)!

[am11]

I agree. This proves two things: 1) libsass API documentation is very reliable and 2) @matryo is an excellent C++ developer!

[am11]

Wouldn't it make sense to take initPrototype and get_constructorName definitions to cpp as well?

Same applies to CoreValue class.

[matryo]

Absolutely. My rationale for leaving them in the header was better readability of what is made available to the JS runtime but moving them to the cpp makes a lot of sense as well.

[am11]

You have already made the code much readable than it was before. IMO with this, it will make the code discoverability much easy for future contributors.

My vote is for even making code/implementation files separate for each derived classes (like you did for create_string). :)

[coveralls]

Coverage increased (+0.69%) to 93.0% when pulling d6ce2d4 on matryo:custom_functions into 98f300e on sass:master.

[matryo]

I pushed a few commits that should cover all of the recommendations made thus far, namely:

• Removed the importer wrap in renderSync()
• Reorganized binding.gyp
• Split sass types into separate modules
• Use of constexpr static variables in CoreValue subclasses in lieu of a static function
• Added support for potentially omitting function signature, used @mgreter's suggestion of using fn(...)

Also made some improvements on the following points:

• Made the sass types' constructors more flexible so they can be called without the new keyword
• Errors not caught by custom functions are now properly bubbled up to libsass by returning SASS_ERROR values.

As far as the API doc for libsass is concerned, I concur it is really nice! Unlike a lot of API docs it does not beat around the bush and goes straight to the point, I found it very useful during this exercise. So great job!

[am11]

Excellent job there @matryo! And thanks for adding the alternative syntax . I will test it and give you further feedback. By it looks, it seems very promising as is. :)

[chriseppstein]

@am11

Thirdly, I am not sure about the new Sass types you added (new sass.types.String('h' + i))). Are these arbitrary types? In general we do not add additional syntaxes in node-sass beyond what libsass has to offer. Especially not to solely meet the requirement of a separate framework such as Compass, which is beyond the scope of this project.

New types are very much in the scope of node-sass. Without them, how would you return a sass value? Some js types could be auto-boxed into Sass values but many can not. In ruby sass, these types are part of the core sass project. E.g. https://github.com/sass/sass/blob/stable/lib/sass/script/value/number.rb

[am11]

Without them, how would you return a sass value?

I thought these will return cstrings from our binding and LibSass will take care of the rest. Unlike ruby-sass, node-sass is only a wrapper around libsass. Besides, I do not know about the use-case when the custom functions are required to return anything but the stringly typed value, which LibSass will eventually stamp out to resultant CSS as is?

[matryo]

@am11 From what I understood of the API, custom functions registered with sass_option_set_c_functions() are expected to return a pointer to a Sass_Value struct, which has to be of the adequate type. Meaning if the function advertises that it returns a color, it should return a Sass_Value that was created with sass_make_color(). If it were to return a string (i.e. a Sass_Value that was created with sass_make_string()), the return value could not be re-injected into a function that expects a color as its argument (e.g. lighten()) making our custom functions hardly composable, i.e. it would be impossible to do something like:

div {
    color: lighten(foo(), 30%);
}

...if foo returned a string.

As I said earlier we could however let the end developer return strings and have a post-processing stage (either in index.js or in binding land) that detects patterns (length literals, color literals, etc.) and automagically transform them into the adequate type.

[am11]

@matryo, that makes sense. :)
I was wondering if we can make custom functions implementer underlying sass-type agnostic, unless it is utterly necessary (which it is), so to avoid learning curve overhead.

that detects patterns (length literals, color literals, etc.) and automagically transform them into the adequate type.

I think that won't be necessary. With the proper documentation and tests cases to exemplify the usage (as you have already added both 👍 ), the implementers of node-sass API shall be able to define custom functions without much of a hassle. Since the main advantage of node-sass is performance; I am afraid pattern sniffing with regex will have a negative affect on perf ?

[chriseppstein]

As one of the heaviest users of the Ruby function api that is provided by Sass, this patch is exactly what I was expecting. returning javascript objects that are converted to Sass_Values is exactly what any developer coming from the Ruby environment would have expected.

[mgreter]

I can only support @chriseppstein words! This is exactly as we did it in perl-libsass. As others and I already have mentioned, there is some room for further improvements to i.e. make maps (objects), list (arrays) and other types (strings) feel more native in a js environment.
And this patch is IMO a good starting point for that!
Edit: perl-libsass docu on Sass_Values can be found at:
https://metacpan.org/pod/CSS::Sass::Type

[chriseppstein]

A few other things I was expecting to see here:

1. Sass options

I would expect this.options to be an object containing the current sass compilation options. Additionally, it should contain some runtime values like the current css output file and the current sass file and the top level sass file. These are critical to writing functions that can interact with the filesystem and handle URL generation correctly.

1. Sass environment

In ruby, you can access the current selector context I would expect this.environment.selector to be accessible as long as libsass has the script-parent feature.

You can also access the current variables defined in the global scope this.globalVariables is essential to being able to re-use sass configuration in the javascript language just like a sass-defined function can.

1. Calling to other Sass functions

It is very useful to be able to invoke sass builtin functions from within a custom function. It's also nice to be able to invoke functions defined in sass. For example, the global-variable-exists function that is a built-in in Sass should be able to tell us if some global is defined. I would expect this.environment.call('global-variable-exists', sass.types.String("foo")) or something similar to work.

Obviously what we have is a good first step, and I wouldn't block landing this PR because these features are missing (Ruby Sass didn't have them at first either)

[am11]

@chriseppstein, (sorry for this getting stretched and GH has zero comment threading/nesting) here:

As one of the heaviest users of the Ruby function api that is provided by Sass, this patch is exactly what I was expecting. returning javascript objects that are converted to Sass_Value s is exactly what any developer coming from the Ruby environment would have expected.

do you mean return value's type of custom function should be specified by user as @matryo's solution (sass.types.blah) or should the type be programatically inferred from returned regular JS object by our API or binding code?

[matryo]

@chriseppstein You raised three very interesting points. At the moment it unfortunately looks like libsass' public API cannot provide what we would need to implement something like this. After looking at libsass' internals I can see that there would be a rather straight forward way to implement those features, but that would be using a private API, which is a bit questionable I guess.

I will take some time this week to augment the JS binding objects so as to provide a more user friendly API.

[xzyfer]

@matryo feel free to PR Libsass with the additional information you require. I'm sure @mgreter will happily assist you with anything you need.

[chriseppstein]

@am11

do you mean return value's type of custom function should be specified by user as @matryo's solution (sass.types.blah) or should the type be programatically inferred from returned regular JS object by our API or binding code?

I meant that it should be sass.types.Blah.

[am11]

@matryo see #783.

[am11]

🎉

It is official now!! ☀️ 🌟 ⭐ ♠️ 💯

Brilliant work @matryo 👍

[callumlocke]

👍👍👍 awesome

[am11]

@matryo, do we need Boolean and Null exported twice: separately as types exports.FALSE, exports.TRUE, exports.NULL and then nested under types as exports.types.Boolean.TRUE, exports.types.Boolean.FALSE, exports.types.NULL?

[chriseppstein]

❤️ You guys rock my socks off.

[Snugug]

I second his ❤️

[jakobo]

🎉 Awesome! ❤️

[jenbennings]

[matryo]

@am11 They indeed do not necessarily have to be on both and we could refactor the code to have those singleton references exported just once. It just felt easier to instantiate those singletons within Boolean::get_constructor and Null::get_constructor and attach them to the constructor object, and copy those references to module.exports in a second pass for convenience. I guess we could also just leave it as is and just not advertise the exports.types.Boolean.TRUE, exports.types.Boolean.FALSE, exports.types.NULL references.