Precedence and identification of promises, streams, and event emitters

[chocolateboy]

Node.js: v7.4.0 async-done: 1.2.2 gulp: gulpjs/gulp#4.0 cp-file: 4.1.1

cc @sindresorhus

Event emitters aren't (always) streams. Eliding them (the documentation says "Stream or EventEmitter returned") causes surprising behaviour if the return type is a) a promise, b) an event emitter, and c) not a stream.

The cp-file module's copy function returns an object which is both a promise and an event emitter. The latter allows it to be monitored for progress updates when copying large files:

copy(src, dest)
    .on('progress', ...)
    .then(...)

async-done appears to treat it as a stream because of the on method, rather than a promise i.e this doesn't work:

gulpfile.js

import copy from 'cp-file'
import gulp from 'gulp'

gulp.task('copy', () => copy('foo.txt', 'bar.txt'))

output

$ gulp copy
Using gulpfile ~/temp/copy-file-test/gulpfile.js
Starting 'copy'...
The following tasks did not complete: copy
Did you forget to signal async completion?

- but this works:

gulpfile.js

import copy from 'cp-file'
import gulp from 'gulp'

gulp.task('copy', () => {
    let promise = copy('foo.txt', 'bar.txt')
    delete promise.on
    return promise
})

output

$ gulp copy
Using gulpfile ~/temp/copy-file-test/gulpfile.js
Starting 'copy'...
Finished 'copy' after 12 ms

An obvious solution would be to check if the return value is a promise before checking if it's an event emitter. Better still, why not use is-stream to check if it's actually a stream? (I'm not sure what the priority should be if the value is both a promise and a (real) stream.)

If the event-emitter-as-terminable (AKA endable/closeable/finishable &c.) interpretation is really needed, it seems like it should be the last resort, when all else fails. But is it actually needed? I can think of one example of an event emitter which is terminable, but which isn't a stream — i.e. a child process — but that appears to be handled separately. Are there any other examples?

(Apologies if this isn't the right place to report this. The layers of abstraction underpinning gulp are impressive and impeccable until you need to figure out where a problem lies, at which point they become a twisty and seemingly-interminable :smile: maze: cp-file -> gulp (v4) -> bach -> undertaker -> async-done -> ???)

[phated]

Thanks for the report; however, this is a problem with the module you are using. The event emitter interface should not overlap with the promise interface. I once asked the creators of Promises/A+ if I could tack the then method onto an interface and they told me it was very bad practice.

[schnittstabil]

The event emitter interface should not overlap with the promise interface. I once asked the creators of Promises/A+ if I could tack [?] the then method onto an interface and they told me it was very bad practice.

Sadly, I can't follow you on this. Promises/A+ uses the term thenable, thus if (typeof p.then === 'function') is the right way, isn't it?

As far as I can see, there is no problem for async-done to change the test order:

-    if (result && typeof result.on === 'function') {
-      // Assume node stream
-      d.add(result);
-      eos(exhaust(result), eosConfig, done);
-      return;
-    }

    if (result && typeof result.subscribe === 'function') {
      // Assume RxJS observable
      result.subscribe(onNext, onError, onCompleted);
      return;
    }

    if (result && typeof result.then === 'function') {
      // Assume promise
      result.then(onSuccess, onError);
      return;
    }

+    if (result && typeof result.on === 'function') {
+      // Assume node stream
+      d.add(result);
+      eos(exhaust(result), eosConfig, done);
+      return;
+    }

Which would be the right order in my opinion anyway.

[schnittstabil]

Furthermore, the following change using Stream.prototype.pipe would make much more sense too:

-    if (result && typeof result.on === 'function') {
+    if (result && typeof result.pipe === 'function') {

[phated]

@schnittstabil It is by design that EventEmitters are supported like streams and is documented as such. This allows child_processes to be returned. The order is desired.

The spec says that something is thenable if that check is true; however, it doesn't speak to the philosophy of promises, which I discussed with the creators in chat and they said that attaching a .then method to any old interface was incorrect design.

I'm going to lock this so I don't have to keep repeating myself.