Custom importer fails silently if you return a buffer

[callumlocke]

With custom importers, it seems you have to return the file contents as a string. If you return a buffer, then node-sass acts as if you didn't return any contents at all, i.e. it reads the file from disk (bypassing any preprocessing you might have wanted to do in your custom importer). This leads to confusing bugs far away from the cause of the problem.

It would be good if custom importers could return a buffer as well as a string. Alternatively, node-sass could error out when an importer returns contents not as a string

[am11]

This is by design. We keep the (perf-wise expensive) I/O operations to minimum (only in CLI usage) and expect the implementer to read write files or LibSass to deal with reading file contents.

Having said that, the data-type of contents is string and keeping the node-sass tier I/O-agnostic is intentional and better for both parties IMO.

@xzyfer, thoughts?

[am11]

Please also note that we are drifting away from unnecessary intervening between libsass and node-sass implementer. Case in hand: ecae014.

[callumlocke]

I'm not sure I follow you. I'm not suggesting node-sass should get involved in any I/O. I'm just saying it should check the type of whatever contents I return from my custom importer, and throw a useful error if it's not a valid type

[xzyfer]

I agree that node-sass should fail fast if a custom importer is given bad
data.
On 13 Feb 2015 09:06, "Callum Locke" notifications@github.com wrote:

I'm not sure I follow you. I'm not suggesting node-sass should get
involved in any I/O. I'm just saying it should check the type of whatever
contents I return from my custom importer, and throw a useful error if it's
not a valid type

—
Reply to this email directly or view it on GitHub
#670 (comment).

[am11]

I thought your initial suggestion was node-sass to convert the fs buffer to string?

Keeping node-sass as dumb as possible and letting LibSass originating errors is a good thing. Checks like these bring complexity and more features requests for things which node-sass shouldn't be doing.

You are seeing it as one simple feature. If you think of it from the angle of all those new sass types which custom functions feature is bringing and if we implement type checking pretty errors for importers, users will be asking for type validation for custom functions as well. Then some may ask the way to customise the error messages and then something more. Yes we can definitely do all that in JS as a language but node-sass primary role is to act like a sleek transparent wrapper not the extension to libSass.

[callumlocke]

I thought your initial suggestion was node-sass to convert the fs buffer to string?

Yes, but I understand you don't want to coerce it, and I'm happy to just call done({contents: buf.toString()}) myself.

Keeping node-sass as dumb as possible and letting LibSass originating errors is a good thing.

I totally agree in general, but the problem here is libsass doesn't originate any error if you pass it a buffer. It just behaves as if you passed it nothing, in which case, by design, it reads the file again from disk (this time bypassing your importer). So everything appears to be working fine – your importer is getting invoked, and the Sass is building without errors, but really it's not working at all. It's a very sneaky bug, that's why I think it's worth doing a quick type check in this particular case, i.e.

if (data.contents && typeof data.contents !== 'string') {
  throw new TypeError('Expected contents to be a string');
}

[am11]

@callumlocke, we will consider it after custom functions are implemented. There might be other changes to importer in LibSass implementation as well: #651. The idea would be to devise a best strategy which would work for both custom importers and functions.

Thanks for your feedback!

[am11]

Fixed by 69f8f4b via #833.