renderSync vs async custom importers

[ghost]

Original problem

A callback function was not being passed on to custom importers when using renderSync. Also renderSync is not set up to stop and wait for an importer to complete async operation before moving on with the render.

The original issue #997

Proposal

• Ensure done is always supplied to importers and handle firing the callback if it is called synchronously (doesn't actually have to wait for anything)
• Emit an error if an importer doesn't return (or syncly callback) any results when in renderSync mode (assume it tried to do something it's waiting for).
• Supply a mode flag indicating either 'sync' or 'async' to importers on the options object to allow importers to decide how to execute in current context.

Under this scheme, eyeglass's importer could look something like this (providing it's important for it to read files asyncly). Basically always calling done() to complete and deciding elsewhere in the file to use readFile() or readFileSync.

Alternatives

• Maybe use something like deasync to hold execution and wait for async importers to return.
• I'm missing some way to make async work?
• Maybe there is no reason to do anything async in an importer?

[xzyfer]

I'm not a fan of this error message. Given the use case for this message to appear there isn't much the user can do about it.

We should provide enough context for the user to action on this error message. At a minimum we should should supply the file that failed to import.

[xzyfer]

All in all I like where this going. Great work @DominicBarnard

Next steps I see

• the error handling needs work. You can see my feelings about developer UX in Fix #924: Empty data string is not a file name #956 (comment)
• the git history needs some love. I think you can just squash this PR to a single commit.

[xzyfer]

@am11 @saper I like your thoughts on this PR

[xzyfer]

Maybe there is no reason to do anything async in an importer

I have thought about this as well. We currently have issues with async importers due to node's threadpool limit #857. Going sync has some performance implications but it entirely works around this issue.

Maybe @chriseppstein has some practical thoughts here from his eyeglass work.

[am11]

The idea of renderSync is to return instead of calling done(). If you want to wait, you need render (async). If you have other use-case, which requires synchronous op, you use renderSync through and through.

IMHO, we shouldn't mix the expected behavior of renderSync to accommodate callback, when we have async functionality available.

[ghost]

@am11 I agree that using renderSync through and through is preferred, which works fine when you are creating your own custom importer to work with your own project. The concern is the added complexity of handling both sync and async scenarios when creating 3rd party importers like eyeglass.

For example here is the eyeglass importer when it originally had to handle this with both return and done callback.

Notice there is a lot of

if(done){
  ...
}else{
  return ...
}

You can compare that to my suggested version of the same importer implemented against this PR.
There you can see that the importer function can simply always exit with callback and use this.options.mode to differentiate at the point of choosing between sync and async ops.

I suppose the difference is a matter of preferred style. Personally i tend towards offering flexibility to the user.

[am11]

@DominicBarnard, I totally get the sentiment "why not that option as well".

Please see: #530 (comment)

The reality is, I initially implemented the custom importers with unified sync and asyn signatures (and a unified base), but after spending days investigating the weird/undefined behaviors it turned out the rabbit hole goes much deeper. v8 inherently binds with libuv in a way that you cannot await the synchronous call (not with uv or C++ mutices). Someone told me that it is not possible on libuv's IRC channel, but I still managed to learn it the hard way. :)

With that said, yes we can pass the done cb to renderSync at the risk of getting misused by the consumer. Since we have discussed the pros and cons, I will leave the final decision to @xzyfer.

[maoberlehner]

What is the current status of this issue? I created some custom importers (most notably: https://github.com/maoberlehner/node-sass-magic-importer) and noticed the problem today. Should I rewrite my custom importer packages to work synchronously or wait for a fix in node-sass?

[saper]

@maoberlehner I don't believe this is going to be changed. Just use render() for the async callbacks and renderSync() if you expect the renderer to wait.

I think we should change the way the C++ bridge behaves. There is too much JavaScript wrapping the C++ bridge already, I think we should simplify it and by the way provide a better importer interface.

It is unlikely I'll find time to work on the bridge until the end of this year, though.