Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Custom functions #644

Merged
merged 1 commit into from
Mar 23, 2015
Merged

Custom functions #644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ matrix:
before_install:
- sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test
- sudo apt-get update
- sudo apt-get install gcc-4.8 g++-4.8
- sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-4.8 20
- sudo update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-4.8 20
- sudo apt-get install gcc-4.7 g++-4.7
- sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-4.7 20
- sudo update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-4.7 20
- g++ --version
- sudo apt-get update -qq
- git submodule update --init --recursive
Expand Down
62 changes: 62 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,68 @@ When returning or calling `done()` with `{ contents: "String" }`, the string val

`this` refers to a contextual scope for the immediate run of `sass.render` or `sass.renderSync`

### functions
`functions` is an `Object` that holds a collection of custom functions that may be invoked by the sass files being compiled. They may take zero or more input parameters and must return a value either synchronously (`return ...;`) or asynchronously (`done();`). Those parameters will be instances of one of the constructors contained in the `require('node-sass').types` hash. The return value must be of one of these types as well. See the list of available types below:

#### types.Number(value [, unit = ""])
* `getValue()`/ `setValue(value)` : gets / sets the numerical portion of the number
* `getUnit()` / `setUnit(unit)` : gets / sets the unit portion of the number

#### types.String(value)
* `getValue()` / `setValue(value)` : gets / sets the enclosed string

#### types.Color(r, g, b [, a = 1.0]) or types.Color(argb)
* `getR()` / `setR(value)` : red component (integer from `0` to `255`)
* `getG()` / `setG(value)` : green component (integer from `0` to `255`)
* `getB()` / `setB(value)` : blue component (integer from `0` to `255`)
* `getA()` / `setA(value)` : alpha component (number from `0` to `1.0`)

Example:

```javascript
var Color = require('node-sass').types.Color,
c1 = new Color(255, 0, 0),
c2 = new Color(0xff0088cc);
```

#### types.Boolean(value)
* `getValue()` : gets the enclosed boolean
* `types.Boolean.TRUE` : Singleton instance of `types.Boolean` that holds "true"
* `types.Boolean.FALSE` : Singleton instance of `types.Boolean` that holds "false"

#### types.List(length [, commaSeparator = true])
* `getValue(index)` / `setValue(index, value)` : `value` must itself be an instance of one of the constructors in `sass.types`.
* `getSeparator()` / `setSeparator(isComma)` : whether to use commas as a separator
* `getLength()`

#### types.Map(length)
* `getKey(index)` / `setKey(index, value)`
* `getValue(index)` / `setValue(index, value)`
* `getLength()`

#### types.Null()
* `types.Null.NULL` : Singleton instance of `types.Null`.

#### Example

```javascript
sass.renderSync({
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;
}
}
});
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is time to maintain a Wiki for node-sass. Lets refactor the README as well. 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, thank you for working on this. It's super important functionality.

The property setters in these Sass value objects are very worrisome to me. Sass values are immutable. If this makes it possible to mutate the Sass values created from a sass file then, it's a violation of the language's design. If it doesn't then this makes it seem like you can, and would make it hard for them to debug what is going on when it doesn't work. My preference would be to make these objects completely immutable, but barring that, I think the objects passed into the function from the sass compiler should be marked as "readonly" and have any attempt to use the setters result in an error. After looking into the code more closely it seems that you're just binding libsass value objects to javascript here so this critique may be more towards libsass than the javascript binding layer. (ping @akhleung)

A few more things that we've done in the ruby api (can totally be added after this lands):

  • null, true and false are always the same (immutable) object.
  • All objects can be cast to a boolean to test if they are truthy or falsey. Only null and false ever return false.
  • These objects should have a toSass() or toString() that returns a string representation for debugging.
  • Maps should have nth($index) function that returns the key/value pair at the nth index. If they are mutable, setters, inserts, deletes by index make sense too.
  • Numbers should have a comparator that does the css unit conversions if need be.
  • All Sass values should support an equality check that returns true iff the sass == operation would return true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the Map type should have a keys method. How else can you iterate over the map?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chriseppstein This is great input! You are right, the SassTypes::Value objects exposed to the v8 runtime constitute a very thin binding layer to the corresponding Sass_Value objects from libsass and as such expose all the same setters.

That said, custom functions are invoked with their arguments passed by value (i.e. SassTypes::Value’s constructor performs a deep copy of the underlying Sass_Value object via sass_clone_value) and similarly the results are returned by value (see SassTypes::Value::get_sass_value()). As a consequence it would not be possible for a custom function to mutate sass values created from a sass file. This immutable virtue is thus preserved for all values from the perspective of the sass files.

I understand that Sass as a language only exposes immutable values and as such it would make a lot of sense to hold onto this philosophy when it comes to custom functions especially if you guys think this would relieve ambiguities potentially perceived by the end developer.

However, If I can offer my two cents as a developer who spends a good deal of time working with JS: this is by no means a necessity. The fact that Sass’ design does not favor mutable values does not have to bleed onto the conventions established in Javascript. While I can totally make sense of immutable strings and numbers, I believe immutable maps and lists would feel awkward to a lot of JS developers out there. Additionally making these complex types immutable may lead to inferior performance results (and I said “may” because I have not looked at what’s behind the public libsass API, so I couldn't affirm that for sure).

In any case, I believe you are absolutely right in pushing for more user-friendly objects. And while it would make sense to me to leave all the subclasses of SassTypes::Value strictly as a translation layer, I can totally see a variety of facade objects in the Javascript realm that would expose all the goodies that you mentioned.

As far as the Map type is concerned, you would iterate over the map the same way you would if you had to use libsass directly:

function(myMap) {
    var i, l, key, value;

    for (i = 0, l = myMap.getLength(); i < l; i++) {
        key = myMap.getKey(i);
        value = myMap.getValue(i);
    }
}


### includePaths
Type: `Array<String>`
Default: `[]`
Expand Down
69 changes: 40 additions & 29 deletions binding.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,19 @@
'target_name': 'binding',
'sources': [
'src/binding.cpp',
'src/sass_context_wrapper.cpp'
'src/create_string.cpp',
'src/custom_function_bridge.cpp',
'src/custom_importer_bridge.cpp',
'src/sass_context_wrapper.cpp',
'src/sass_types/boolean.cpp',
'src/sass_types/color.cpp',
'src/sass_types/error.cpp',
'src/sass_types/factory.cpp',
'src/sass_types/list.cpp',
'src/sass_types/map.cpp',
'src/sass_types/null.cpp',
'src/sass_types/number.cpp',
'src/sass_types/string.cpp'
],
'include_dirs': [
'<!(node -e "require(\'nan\')")',
Expand All @@ -15,41 +27,39 @@
'libsass.gyp:libsass',
]
}],
['libsass_ext == "auto"', {
'cflags_cc': [
'<!(pkg-config --cflags libsass)',
],
'link_settings': {
'ldflags': [
'<!(pkg-config --libs-only-other --libs-only-L libsass)',
],
'libraries': [
'<!(pkg-config --libs-only-l libsass)',
],
}
}],
['libsass_ext == "yes"', {
'cflags_cc': [
'<(libsass_cflags)',
],
'link_settings': {
'ldflags': [
'<(libsass_ldflags)',
],
'libraries': [
'<(libsass_library)',
],
}
['libsass_ext == "auto"', {
'cflags_cc': [
'<!(pkg-config --cflags libsass)',
],
'link_settings': {
'ldflags': [
'<!(pkg-config --libs-only-other --libs-only-L libsass)',
],
'libraries': [
'<!(pkg-config --libs-only-l libsass)',
],
}
}],
['libsass_ext == "yes"', {
'cflags_cc': [
'<(libsass_cflags)',
],
'link_settings': {
'ldflags': [
'<(libsass_ldflags)',
],
'libraries': [
'<(libsass_library)',
],
}
}],
['OS=="mac"', {
'xcode_settings': {
'OTHER_CPLUSPLUSFLAGS': [
'-std=c++11',
'-stdlib=libc++'
],
'OTHER_LDFLAGS': [
'-stdlib=libc++'
],
'OTHER_LDFLAGS': [],
'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',
'GCC_ENABLE_CPP_RTTI': 'YES',
'MACOSX_DEPLOYMENT_TARGET': '10.7'
Expand All @@ -73,6 +83,7 @@
}],
['OS!="win"', {
'cflags_cc+': [
'-fexceptions',
'-std=c++0x'
]
}]
Expand Down
119 changes: 112 additions & 7 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,67 @@ function getOptions(options, cb) {
return options;
}

/**
* Executes a callback and transforms any exception raised into a sass error
*
* @param {Function} callback
* @param {Array} arguments
* @api private
*/

function tryCallback(callback, args) {
try {
return callback.apply(this, args);
} catch (e) {
if (typeof e === 'string') {
return new binding.types.Error(e);
} else if (e instanceof Error) {
return new binding.types.Error(e.message);
} else {
return new binding.types.Error('An unexpected error occurred');
}
}
}

/**
* Normalizes the signature of custom functions to make it possible to just supply the
* function name and have the signature default to `fn(...)`. The callback is adjusted
* to transform the input sass list into discrete arguments.
*
* @param {String} signature
* @param {Function} callback
* @return {Object}
* @api private
*/

function normalizeFunctionSignature(signature, callback) {
if (!/^\*|@warn|@error|@debug|\w+\(.*\)$/.test(signature)) {
if (!/\w+/.test(signature)) {
throw new Error('Invalid function signature format "' + signature + '"');
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has history attached to it. Without this, it fails with CLI usage and manually calling render with importer. If you have discovered a way to fix it, then Kudos! But please make sure of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I must have removed this thinking this was part of my debug logs. But now I'm intrigued, is there an issue ticket that explains why this hack was introduced?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes here: #615

If anyone could ever explain this!

I was also intrigued about it. Took me sometime to devise this random hack. :D

It could have fixed in this code overhaul of yours. Nonetheless, I am still very interested to know about what was causing it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see. Well I am glad to announce that yes this indeed got fixed along with my overhaul :D which I guess is a good thing since the previous hack would not play nice with the --stdout option of the cli.

The reason this was happening is because of missed wakeup, i.e. the notify() call executed before the wait() call, leaving wait() hang indefinitely. To work around this, wait() has to be guarded by a loop that verifies whether the condition that we are waiting for may have already been met. Because that's a very common pattern wait() can actually receive an optional predicate as its second argument that does just that. That's how I used it:

matryo@8fdf5c4#diff-953483bd71622742606ddb26e23f1e62R103

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant work! and thanks for explaining it!! 💯

We were using uv_mutex and it was later we switched to std mutex. I had no idea that we can check already semaphored cond var in predicate. :D

Everyday learning new things on GitHub. :)

return {
signature: signature + '(...)',
callback: function() {
var args = Array.prototype.slice.call(arguments),
list = args.shift(),
i;

for (i = list.getLength() - 1; i >= 0; i--) {
args.unshift(list.getValue(i));
}

return callback.apply(this, args);
}
};
}

return {
signature: signature,
callback: callback
};
}

/**
* Render
*
Expand Down Expand Up @@ -172,13 +233,9 @@ module.exports.render = function(options, cb) {
var importer = options.importer;

if (importer) {
options.importer = function(file, prev, key) {
options.importer = function(file, prev, bridge) {
function done(data) {
console.log(data); // ugly hack
binding.importedCallback({
index: key,
objectLiteral: data
});
bridge.success(data);
}

var result = importer.call(options.context, file, prev, done);
Expand All @@ -189,6 +246,31 @@ module.exports.render = function(options, cb) {
};
}

var functions = options.functions;

if (functions) {
options.functions = {};

Object.keys(functions).forEach(function(signature) {
var cb = normalizeFunctionSignature(signature, functions[signature]);

options.functions[cb.signature] = function() {
var args = Array.prototype.slice.call(arguments),
bridge = args.pop();

function done(data) {
bridge.success(data);
}

var result = tryCallback(cb.callback, args.concat(done));

if (result) {
done(result);
}
};
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, we need to reuse the same logic for CLI usage. Just note for self. Let me know if / whenever you are adding the CLI support (before that). We need to adapt and enhance on the way jscs, jshint and other linters support custom importers. I will then explain it. :)


options.data ? binding.render(options) : binding.renderFile(options);
};

Expand All @@ -206,10 +288,24 @@ module.exports.renderSync = function(options) {

if (importer) {
options.importer = function(file, prev) {
return { objectLiteral: importer.call(options.context, file, prev) };
return importer.call(options.context, file, prev);
};
}

var functions = options.functions;

if (options.functions) {
options.functions = {};

Object.keys(functions).forEach(function(signature) {
var cb = normalizeFunctionSignature(signature, functions[signature]);

options.functions[cb.signature] = function() {
return tryCallback(cb.callback, arguments);
};
});
}

var status = options.data ? binding.renderSync(options) : binding.renderFileSync(options);
var result = options.result;

Expand All @@ -228,3 +324,12 @@ module.exports.renderSync = function(options) {
*/

module.exports.info = process.sass.versionInfo;

/**
* Expose sass types
*/

module.exports.types = binding.types;
module.exports.TRUE = binding.types.Boolean.TRUE;
module.exports.FALSE = binding.types.Boolean.FALSE;
module.exports.NULL = binding.types.Null.NULL;
6 changes: 2 additions & 4 deletions libsass.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,12 @@
'-std=c++11',
'-stdlib=libc++'
],
'OTHER_LDFLAGS': [
'-stdlib=libc++'
],
'OTHER_LDFLAGS': [],
'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',
'GCC_ENABLE_CPP_RTTI': 'YES',
'MACOSX_DEPLOYMENT_TARGET': '10.7'
}
}],
}],
['OS=="win"', {
'msvs_settings': {
'VCCLCompilerTool': {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"coverage": "node scripts/coverage.js",
"install": "node scripts/install.js",
"postinstall": "node scripts/build.js",
"pretest": "node_modules/.bin/jshint bin lib test",
"pretest": "node_modules/.bin/jshint bin lib scripts test",
"test": "node_modules/.bin/mocha test"
},
"files": [
Expand Down
6 changes: 3 additions & 3 deletions scripts/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function afterBuild(options) {
*/

function build(options) {
var arguments = [
var args = [
path.join('node_modules', 'pangyp', 'bin', 'node-gyp'),
'rebuild',
].concat(
Expand All @@ -63,9 +63,9 @@ function build(options) {
})
).concat(options.args);

console.log(['Building:', process.sass.runtime.execPath].concat(arguments).join(' '));
console.log(['Building:', process.sass.runtime.execPath].concat(args).join(' '));

var proc = spawn(process.sass.runtime.execPath, arguments, {
var proc = spawn(process.sass.runtime.execPath, args, {
stdio: [0, 1, 2]
});

Expand Down
Loading