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

Render options in callbacks as this.options #686

Merged
merged 1 commit into from
Feb 20, 2015
Merged

Render options in callbacks as this.options #686

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
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ Note: If this option is provided to renderSync it will be ignored. In case of `r
#### importer (starting from v2)
`importer` is a `Function` to be called when libsass parser encounters the import directive. If present, libsass will call node-sass and let the user change file, data or both during the compilation. This option is optional, and applies to both render and renderSync functions. Also, it can either return object of form `{file:'..', contents: '..'}` or send it back via `done({})`. Note in renderSync or render, there is no restriction imposed on using `done()` callback or `return` statement (dispite of the asnchrony difference).

The options passed in to `render` and `renderSync` are available as `this.options` within the `Function`.

#### includePaths
`includePaths` is an `Array` of path `String`s to look for any `@import`ed files. It is recommended that you use this option if you are using the `data` option and have **any** `@import` directives, as otherwise [libsass] may not find your depended-on files.

Expand Down
12 changes: 7 additions & 5 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ function getSourceMap(options) {
* @param {Object} options
* @api private
*/

function getOptions(options) {
options = options || {};
options.comments = options.source_comments || options.sourceComments || false;
Expand All @@ -140,6 +139,9 @@ function getOptions(options) {
options.sourceMap = getSourceMap(options);
options.style = getStyle(options) || 0;

// context object represents node-sass environment
options.context = { options: options };

if (options.imagePath && typeof options.imagePath !== 'string') {
throw new Error('`imagePath` needs to be a string');
}
Expand All @@ -149,7 +151,7 @@ function getOptions(options) {

options.error = function(err) {
if (error) {
error(util._extend(new Error(), JSON.parse(err)));
error.call(options.context, util._extend(new Error(), JSON.parse(err)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just say error.call(options, ...) and remove options.context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that would be a good idea. It would paint us into a bit of a corner if we need to surface any additional environment information from libsass or node-sass to callbacks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are not cloning options on context instantiation, I think one problem with this approach would be the shallow copy dropping functions definitions (importer et al.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry @am11, I don't think I understand here. I wrote a test locally to see if anything was being dropped from the options object or we were somehow losing our function definitions.

it('should preserve options by reference', function(done) {
  var options;
  options = {
    data: src,
    success: function() {
      assert.strictEqual(this.options, options);
      assert.strictEqual(this.options.importer, options.importer);
      done();
    },
    importer: function() {
      return {
        contents: 'div {color: yellow;}'
      };
    }
  };
  sass.render(options);
});

Because we are mutating objects by reference, everything seems to arrive as expected in the callbacks. Should I change something in the above test to capture your concerns? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice! I thought it will drop those. Thanks for the example! (we can add this test to the mix as well) 👍

Separately, are you suggesting if we drop options.context, then any new member to options set by node-sass will be lost in callbacks? Can you please provide a concrete example for this, which shows that it will fail on some account in case we drop .call(options.context, ...) in favour of .call(options, ...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

I'm worried about the case where we write this as importer.call(options, ...). For this, options such as file will be accessible as this.file. It will work as long both libsass and node-sass do not need to share any additional information with custom importers and functions.

In ruby-sass which libsass seeks parity with, there is a concept of a global environment. I can see us providing this in a more node-like way as opposed to the ruby pattern of creating a global. If we are currently binding to options, then we would be unable to add this environment information without a breaking change (as references to this.file would suddenly fail). The context object fulfills this role, allowing us to later add environment information { options: options, env: getEnvironment() } without breaking anyone's code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks and points for future consideration. 👍

TBH, I haven't delve into global environment, I cannot say for a fact whether it will land in libsass world or not. Maybe @xzyfer can confirm it.

However, I do think that it will not break the user code in this case even if global environment is introduced in libsass. This is because whatever libsass is sharing with node-sass, it arrives via the binding layer which uses v8 lib (via nan wrapper). That allows us to assign / inject any object to a persistent (v8 terminology) JS object. So this sharing with custom imports or functions is taken care by the binding code, where we can alter the state of other objects in the current loop (libuv's uv_loop).

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way it does not hurt to keep the context. If you can rebase out the package.json change, we can merge this. :)

}
};

Expand All @@ -158,7 +160,7 @@ function getOptions(options) {
var stats = endStats(result.stats);

if (success) {
success({
success.call(options.context, {
css: result.css,
map: result.map,
stats: stats
Expand Down Expand Up @@ -207,7 +209,7 @@ module.exports.render = function(options) {
});
}

var result = importer(file, prev, done);
var result = importer.call(options.context, file, prev, done);

if (result) {
done(result);
Expand All @@ -232,7 +234,7 @@ module.exports.renderSync = function(options) {

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

Expand Down
73 changes: 69 additions & 4 deletions test/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ describe('api', function() {
}
});
});

it('should override imports with "data" as input and fires callback with contents', function(done) {
sass.render({
data: src,
Expand Down Expand Up @@ -359,6 +359,56 @@ describe('api', function() {
}
});
});

it('should be able to see its options in this.options', function(done) {
var fxt = fixture('include-files/index.scss');
sass.render({
file: fxt,
success: function() {
assert.equal(fxt, this.options.file);
done();
},
importer: function() {
assert.equal(fxt, this.options.file);
return {};
}
});
});

it('should be able to access a persistent options object', function(done) {
sass.render({
data: src,
success: function() {
assert.equal(this.state, 2);
done();
},
importer: function() {
this.state = this.state || 0;
this.state++;
return {
contents: 'div {color: yellow;}'
};
}
});
});

it('should copy all options properties', function(done) {
var options;
options = {
data: src,
success: function() {
assert.strictEqual(this.options.success, options.success);
done();
},
importer: function() {
assert.strictEqual(this.options.importer, options.importer);
return {
contents: 'div {color: yellow;}'
};
}
};
sass.render(options);
});
});

describe('.renderSync(options)', function() {
Expand Down Expand Up @@ -493,7 +543,7 @@ describe('api', function() {
assert.equal(result.css.trim(), '');
done();
});

it('should override imports with "data" as input and returns contents', function(done) {
var result = sass.renderSync({
data: src,
Expand Down Expand Up @@ -521,6 +571,21 @@ describe('api', function() {
assert.equal(result.css.trim(), 'div {\n color: yellow; }\n\ndiv {\n color: yellow; }');
done();
});

it('should be able to see its options in this.options', function(done) {
var fxt = fixture('include-files/index.scss');
var sync = false;
sass.renderSync({
file: fixture('include-files/index.scss'),
importer: function() {
assert.equal(fxt, this.options.file);
sync = true;
return {};
}
});
assert.equal(sync, true);
done();
});
});

describe('.render({stats: {}})', function() {
Expand Down Expand Up @@ -713,8 +778,8 @@ describe('api', function() {
describe('.info()', function() {
it('should return a correct version info', function(done) {
assert.equal(sass.info(), [
'node-sass version: ' + require('../package.json').version,
'libsass version: ' + require('../package.json').libsass
'node-sass version: ' + require('../package.json').version,
'libsass version: ' + require('../package.json').libsass
].join('\n'));

done();
Expand Down