Render options in callbacks as this.options

[jakobo]

Replaces #681 (single focused commit based on sass/node-sass@master)

This patch and tests introduces a context for all callbacks currently
in node-sass. Right now, they run w/ an ambiguous context, making the
this keyword not helpful to importer developers (and eventually
custom function developers). This creates a context object, adds the
render options to said context object, and then uses that context for
importer, success, and error callbacks via this.options.

By using a context instead of just .call(options...) there is a hook
for later should we find additional environmental information is
valuable to importer / custom function developers such as the node
environment, node-sass version, etc.

Additionally, info() is exposed in the context so any node modules
imported would be able to determine the libsass & node-sass versions
used during the render call.

[am11]

Why do we need these three? info and sync are useless IMO, state can be set by user if they need.

//cc @kevva.

[jakobo]

A good example of where these three items are useful is in scenarios where the custom importers (and eventually custom functions) come from npm modules. In these contexts, the importer function probably does not have any information about how it was invoked.

• state: This persistent object allows an importer or custom function to keep track of data across invocations. An example of this would be providing a custom importer that allows for import-once style functionality. I'm happy to remove this if we would prefer to let custom importer authors develop their own solutions
• sync: At the time of the importer function, the importer has no knowledge of if it was invoked via render or renderSync. According to https://github.com/sass/node-sass/blob/master/lib/index.js#L210 there's no easy way to know if this is sync/async. Since it's part of the Sass environment, it made sense to pass it along as part of the context.
• info: It seemed valuable that an importer (or eventually custom function) could ask about the node-sass / libsass versions. I'm okay with removing this if we don't have a clear use case.

[am11]

Thanks for explaining the use-cases.

• We are deep cloning the option object.
• Since the user can do this.options.whatever = { 'bah': 'blah' } to fulfill their custom requirements, they can append all that to options object the way they please.
• The idea really is to keep the API itself less noisier and sleeker.

[jakobo]

That totally makes sense. In ruby sass, there is a custom element and developers are told not to write to the top level hash. http://sass-lang.com/documentation/file.SASS_REFERENCE.html Should we instead rename this to custom for consistency? I do think there's a lot of value in people not writing directly into the top level hash or the option hash.

[am11]

@jakobo,

• node-sass is not complying with ruby-sass, since two runtimes are intrinsically different and we are more focused on node-way or JavaScript-way of doing things here.
• we are not restricting / filtering the options object anyway and so the option object is open-ended. Users are free to consume it.
• info() has no or very obscure usage in importers / custom functions.

I do think there's a lot of value in people not writing directly into the top level hash or the option hash.

1- Why?
2- What is the purpose of having deep-nested clone?

[jakobo]

Totally makes sense. Thanks for explaining the logic and rationale.

For 1) if we don't restrict writing to the options hash, technically any new options we add risk breaking backwards compatibility. We could just tell import / custom function devs to namespace their items stored in the options hash to avoid conflicts.

For 2) importers used in gulp or grunt pipelines use the same options collection repeatedly. It seemed safer to work on a clone of the options. Also happy to pull this out.

More than happy to remove any/all of these items if we think they're outside the goals of node-sass. Just give the word. :)

[am11]

@jakobo, generally these are very good ideas. But after few iterations, you will notice that new use-cases will emerge for the features we initially took lightly. As soon as you try to do something creative here, it may cause chaos in the tertiary, quaternary .. downstream of node-sass.

Therefore, I think it is a good idea to keep the core API side as simple/dumb as possible unless of course it is utterly necessary.

For example, we are even resolving the output/source-map file paths just because libsass require it. And we will be removing the (remaining) default values for options and let libsass set them (#677).

For this PR. I propose to let the consumers:

• use or misuse options object and realise if something goes wrong while misusing it. :)
• clone the options object before passing it to node-sass if they are sharing it elsewhere for other purposes.
• figure out whether the custom function or importer is sync or async if necessary. If they are really defining a function to work with both render[Sync], they can just return the value and it will work for both. For more mysterious use-case (callback for async and return for sync), they are free to override this.options with custom payload.
• make use of this.options in importers, success and error callbacks.

[jakobo]

Based on feedback from @am11, I have removed any additional items from the context object. We can revisit adding them if there are use cases that make it absolutely necessary. The context object is now much more straightforward:

context = { options: options } allows this.options to work in success, error, and importer callbacks. Thanks for the reviews, feedback, and rationale explanations.

[am11]

This is not needed anymore.

[jakobo]

Agree. Will remove. Sorry for missing it. :)

[jakobo]

Latest in the PR @ 2aebbd1 has the package.json changes rebased out. It should be a clean commit now.

[am11]

Excellent! 🎉