Node-sass: Promise support

[MajorBreakfast]

Is there any interest in supporting promises? I'll provide a PR if you're interested. This can be added without introducing an npm dependency by detecting the Promise global via typeof Promise !== 'undefined' and enabling the behavior only if promises are available.

let { css, map } = await sass.render({
  async importer(url, prev) { ... }
})

• For sass.render() if no callback is provided
• For options.importer()

[saper]

Custom importers and functions are still pretty experimental feature. Most importantly they are pretty tightly bound with our C++ implementation. I wonder if this will work at all. Can you sketch a minimal, pure-JS example to test?

[MajorBreakfast]

I'm only mentioning importer() here because it can be async (-> promise) and I'm going to use it in my project.

Basically index.js L322-L324 (and the same some lines above for arrays) need to be modified for this:

if (result) {
   if (result.then) { // New
     result.then(function(r) { done(null, r) }, function(e) { done(e) });
   } else { // Current
     done(result === module.exports.NULL ? null : result);
   }
}

It's a tiny modification: Detects the promise and calls done() when it resolves or rejects with the value or error respectively.

[MajorBreakfast]

Are you OK with this change? I will create the PR with code changes and updates to the README.md if you are.

[xzyfer]

I'm curious to see what am implementation of this would look like. However I'm against only doing this for importers. If we were to support promised we'd have to support custom importers, custom functions, and async render.

[MajorBreakfast]

@xzyfer I agree that it makes most sense to augment all mechanisms that use callbacks in one go.

[xzyfer]

There are no plans to do this in the near future. Callbacks can be promisifed in user land easily enough.

[callumlocke]

Can this be reopened, even if it's not going to be fixed immediately?

@MajorBreakfast's original example is really nice:

let { css, map } = await sass.render({
  async importer(url, prev) { ... }
})

Callbacks can be promisifed in user land easily enough.

It's true that you can do promisify(sass.render). But you can't use promisification to make node-sass understand a promise returned from from an importer function, i.e. there's no way to make the async importer(... thing work, until node-sass supports promises natively.

[xzyfer]

@callumlocke we're open to pull requests with test coverage that also maintain the existing callback API. However the core team has no plans to progress this feature in the foreseeable future.

[callumlocke]

@xzyfer Are you sure you want to retain the old API alongside a new promise-based API? Why not do a major version bump, embrace promises, and simplify the codebase?

The existing callback API is outdated and confusing. The fact you can call done(err) or done(result) (but not done(null, result)) is not even consistent with Node's old conventions, and it provides absolutely no extra functionality compared to promises. Maintaining this old API alongside a new promise API sounds messy (it would involve duck-typing the importer function's return value and/or reading the function's length property to see if it explictly accepts a done argument, neither of which are foolproof). I highly recommend breaking with the past here.

[xzyfer]

@callumlocke

Are you sure you want to retain the old API alongside a new promise-based API?

No, but I am sure I do not wish to stop supporting the callback API. The callback is universally supported.

A significant number of node-sass users are still using node 0.10 and 0.12. This is true for the node ecosystem as a whole. Forcing those users to require a polyfill or --harmony flag is a terrible experience. Where as the callback is universally supported.

The existing callback API is outdated and confusing

True, this unfortunately and predates my involvement with the project. This is some thing we will likely fix in the future however the error or result pattern, although non-standard, is not uncommon and does not justify breaking the ecosystem for.

Maintaining this old API alongside a new promise API sounds messy

True which is why this was originally closed.

[callumlocke]

Forcing those users to require a polyfill or --harmony flag is a terrible experience.`

There would be no need for polyfills or flags. I think you might be confusing the ES6 Promise constructor (added in Node v0.12) with promises in general, which are nothing more than plain JavaScript objects that implement this interface and work in any JavaScript engine with no dependencies.

Anyone who wants to use this new API would just have to change sass.render(opts, callback) to sass.render(opts).then(callback). This code works fine in Node v0.10 (and probably even v0.1) with no polyfills or flags.

does not justify breaking the ecosystem for

What in the ecosystem would break? Wouldn't a semver major version bump solve this?

[MajorBreakfast]

Nice that this as come up again. I think it'd be very simple to have both APIs living side-by-side:

• Detecting an asynchronous function is easy: Just check if it returns an object that has a then()
• Detecting whether a promise is expected as return value is also easy: Just check whether a callback is provided as a parameter. If it is, then use it, else return a promise.

Both things can be added via an if statement. Never did come around to provide PR for this. But it'd be a cool thing to have first-party. Promises really are a thing that you quite enjoy using once you started to.

[xzyfer]

I understand the appeal of promises, but please understand they are a barrier to entry to a lot of new developers. We see promises as an enhancement, not a requirement.

We'll happily consider a promise API as long as

• it works along side the existing callback API
• the implementation isn't overly complicated
• adequate test coverage is provided

[xzyfer]

polygoat looks like it could be a promising solution.

[tomm1996]

any update on this?