Feature request: Async file transformers

[teehemkay]

Unless I'm mistaken, file transformers cannot be async, right?

If that's the case, it's quite unfortunate because it means that one has to chose between async or caching but can't have both?

For example: I want to have a gobble plugin for imagemin. Since optimizing is onerous and a 1-to-1 op, I'd rather use a file transform.

But imagemin processors are async so it's a no go 😞

So this is my request for making file transformers async just like directory transformers. I think it would make gobble much more powerful.

[Rich-Harris]

In principle this makes a lot of sense, and I'm all for it. Tricky part is determining whether it's async or not – returning a Promise is easy enough, but for transformers that are callback-based, adding a third done argument would mean that the transformer was mistaken for a directory transformer.

So, I see a couple of potential solutions:

1) Rather than using the function arity trick, require all plugins to be directory transformers, but have a helper for file transformers...

var fileTransformer = require( 'gobble-file-transformer' );
var myPlugin = fileTransformer( ( input, options ) => {...});

...but I'm not really a fan of this approach for lots of reasons.

2) Never pass a done callback – instead, do something like

function myPlugin ( input, options ) {
  const done = this.async();
  doSomethingWith( input, options, done );
}

So file transformers would continue to have two arguments, and the fourth argument to directory transformers could be gradually phased out.

[fskreuz]

Suggesting dropping callbacks and just use promises to be uniform in returns as well as accommodate both synchronous and asynchronous transforms. Callback-based transforms can just migrate to using promises.

function fileTransform(input, options){.
  return Promise.resolve(stuff); // The usual return values are accepted as resolution.
  return Promise.reject(error);
}

function dirTransform(input, output, options){
  return Promise.resolve(); // Not sure if stuff is needed. We're working directly on the fs.
  return Promise.reject(error);
}

[Rich-Harris]

Trouble is a lot of libraries still use node-style callbacks, and insisting on promises means this sort of nonsense:

function myPlugin ( inputdir, outputdir, options ) {
  return new Promise( function ( fulfil, reject ) {
    legacyBullshit( inputdir, outputdir, options, function ( err, result ) {
      if ( err ) return reject( err );
      fulfil( result );
    });
  });
}

// as opposed to
function myPlugin ( inputdir, outputdir, options ) {
  var done = this.async();
  legacyBullshit( inputdir, outputdir, options, done );
}

// or even
function myPlugin ( inputdir, outputdir, options ) {
  legacyBullshit( inputdir, outputdir, options, this.async() );
}

[teehemkay]

:+1: on proposal 2) above (using this.async). I also think it's premature to go the "promise-only" route.

[MartinKolarik]

:+1: for this.async() for the very same reason @Rich-Harris mentioned above.

[teehemkay]

On second thought (sorry, I'm on vacation), this.async bothers me a little bit: I'd much prefer being explicit that a transformer is async (using a callback)

How about requiring that async file transformers have transformer.defaults.async === true

This way we can still distinguish between a file transformer with a callback and a directory transformer.

[martypdx]

Why not disambiguate and add a different method for file transforms? Async would only be in new method, could be done in non-breaking fashion by logging deprecation warnings for use of .transform as file transformer.

[fskreuz]

So we can have transformFile and transformDirectory and have new implementation on them while keeping the old transform covered with deprecation notices? Sounds good to me 👍 . We can also throw in a Promise+callback hybrid with no issues.

[martypdx]

@fskreuz right, but with @Rich-Harris's supreme naming skills we'd have even cooler, more obvious names like whammo and vamoose ;)

[teehemkay]

Looking at the code, I see that the transform calling context already has other properties (such asthis.log, this.src and this.destamong others which maybe should be mentioned tin the docs?).

So on third thought, this.async makes more sense in this context (no pun intended)

[cprecioso]

While this is being worked out, I thought it'd be nice to have it for Promise-based transforms ASAP, as it is very simple and covers at least a percentage of this use case. Also, for imagemin (the reason @teehemkay created this issue and the reason I ended up here), that works just fine.

The PR is #124.

[teehemkay]

👍

[legomind]

Any chance of this happening?