Custom importer sass.NULL usage

[kenjiqq]

The documentions for custom importers state that
If an importer does not want to handle a particular path, it should return sass.NULL

But if you implement the importer in a package, that will be used as a dependency, this package will have a dependency on node-sass and have node-sass package in it's node_modules.

So when this importer is run from another package, like grunt-sass that has its own dependency on node-sass, the node-sass module used in grunt-sass and in the importer will not be the same object and the object reference equality check in node-sass will fail.

var result = importer.call(options.context, file, prev, done);
//result will be a sass.NULL from another node-sass object and the === fails
if (result) {
    done(result === module.exports.NULL ? null : result);
}

This cause node-sass to think all imports are handled by this importer no matter what.

[kenjiqq]

This is assuming the importer implementation does a require('node-sass') to get a node-sass reference. How else would it be able to access sass.NULL

[xzyfer]

Good questions. Maybe @am11 or @saper would have thoughts here.

[kenjiqq]

In some situation using peerDependencies will solve it, but probably not everywhere

[saper]

Would that help if we would just return undefined? I actually think we should drop sass.NULL and use that native node type, just haven't got around to implementing it yet.

[xzyfer]

@chriseppstein I'd like to hear your thoughts on this since I expect any decision here will impact eyeglass.

[am11]

@kenborge, you have made a very good point and thanks for spotting and reporting this issue.
This issue is in node-sass code itself, which can be hot fixed as:

- done(result === module.exports.NULL ? null : result);
+ done(result.toString() === module.exports.NULL.toString() ? null : result);
// both will return `[object SassNull]` which is different than `[object Object]`

or

- done(result === module.exports.NULL ? null : result);
+ done(result.constructor.toString() === module.exports.NULL.constructor.toString() ? null : result);

The idea was to not advertise these export too much and still able to provide control for explicitly returning SassNull type.

[saper]

@kenborge can you check if my preliminary fix #1308 makes it better? Are you returning your imports vi done( ... ) or by return? If via done(...) then this could be a duplicate of #1296.

[saper]

Actually asking for sass.NULL is a mistake. With #1308 returning JavaScript null should work as well.

[am11]

In JavaScript return null;, return; and returning nothing are interchangeable, which is not applicable here as we need to differentiate between done and return. Explicitly returning sass.NULL is by design.

[saper]

@am11 What about undefined? Unspecified parameter / return value is undefined:

> console.log( ( function() { return; } )() );
undefined
undefined
> console.log( ( function() { return null; } )() );
null

This is what this patch is checking against.

[am11]

@saper, either test the type sass.NULL or drop sass.NULL completely and use return null everywhere (i.e. breaking change). The reason why it wasn't done that (return null and test based on undefined) way in first place is to avoid the confusion and keep the type-systems separate.

[saper]

My patch uses null everywhere and aliases sass.NULL and sass.sass_types.Null.NULL to null for compatibility.

[xzyfer]

A couple things.

if you implement the importer in a package, that will be used as a dependency, this package will have a dependency on node-sass

This is essentially forcing a peer dependency, which I agree is bad.

Actually asking for sass.NULL is a mistake.
Explicitly returning sass.NULL is by design.

I agree that with returning sass.NULL.

In JavaScript return null;, return; and returning nothing are interchangeable

Technically not true.

> function foo() { return null; }
undefined
> function bar() { return; }
undefined
> foo()
null
> bar()
undefined

What about undefined?

We could strictly check against undefined to get around the issue @am11 mentions with return null. @saper's patch does this.

Although technically null is not undefined as @am11 points out they are used interchangeably in the JS community. So much so a lot of developers don't know the conceptual difference. This means we're practically asking for false bug reports due to this misunderstanding.

---

My preference would be move sass-types into it's own repo as discussed in #1088 (comment)

This solves the peerDependency problem, preserves the semantics of returning sass.NULL from importers/functions, and resolves abilities with returning null vs undefined.

[xzyfer]

It's also worth noting that we currently do not distinguish between custom importers returning false, null, undefined, or sass.NULL.

The exception being #1308 where we don't correctly handle return sass.NULL with async importers.

[xzyfer]

I've added test coverage for the existing importer noop return semantics in #1319. This will prevent us from unintentionally introducing BC breaks.

[xzyfer]

TLDR;

@kenborge you can currently return null or undefined to get the same behaviour as sass.NULL in custom importers.

@am11 @saper I've opened #1320 to discussion creating a sass-types package.

[saper]

There are several problems with explicitly using undefined (it can be redefined for example), but we use it only because we want to be nice to those returning via return value instead of done(value) - we intercept that return value for them. It is not a proper solution, but kind of works and checking for undefined fits that ugly pattern.

[saper]

Maybe we would better off by moving this code into the C++ binding. We should know better in C++ if the value was returned and how.

[pmowrer]

@xzyfer Returning null or undefined in place of sass.NULL does not appear to work (3.4.2). The importer will just hang indefinitely after processing a few files whereas it completes using sass.NULL.

[xzyfer]

@pmowrer I believe this has been patched in the current beta. Try npm install node-sass@beta.