promise warnings on latest node

[yocontra]

(node:18540) Warning: a promise was created in a handler at domain.js:121:23 but was not returned from it, see http://goo.gl/rRqMUw
    at Function.Promise.cast (/Users/contra/Projects/staeco/node_modules/unzipper/node_modules/bluebird/js/release/promise.js:195:13)

On latest node. Just using this in a basic way - for each file, pipe it to the fs. Seems like this is caused by this promise being created in autodrain, which I'm not using: https://github.com/ZJONSSON/node-unzipper/blob/master/lib/parse.js#L85

[ZJONSSON]

Thanks @contra. So this is tricky because autodrain returns a promise that could be of use (i.e. resolve when success or reject when error). So I'm not sure that the fix here is to remove the promise altogether.

If you either await the autodrain (inside a function that is async) the warning should go away? Do you have any other suggestions?

[yocontra]

@ZJONSSON Yeah, adding .then(() => null).catch(() => null) will hide the warning but ultimately doesn't solve the issue of a promise hanging around. Maybe have it match the other API and do this?

entry.autodrain = function() {
  __autodraining = true;
  return {
    promise: function() {
      return new Promise(function(resolve,reject) {
        entry.pipe(NoopStream());
        entry.on('finish',resolve);
        entry.on('error',reject);
      });
    }
  };
};

So if you did want to await it, it's await entry.autodrain().promise().

[ZJONSSON]

You still would have to actually pipe the entry into a NoopStream() as the underlying entry is a PullStream, i.e. to get to next entry you need to pull the entire body of the entry you want to autodrain (and just discard the data).

Also if you are not interested in the promise, handling any errors that might occur in autodraining can be problematic (the error event handler is not accessible).

One way to solve this would be to return a stream that passes no data, but includes the error event handlers, but also includes a .promise method.

Something like this:

entry.autodrain = function() {
  __autodraining = true;
  var draining = entry.pipe(NoopStream();
  draining.promise = function() {
    draining.on('finish',resolve);
    draining.on('error',reject);
  };
  return draining;
}

Here you could then do entry.autodrain().on('error', e => dosomething) or entry.autodrain().promise().catch(e => dosomething)

The problem with above suggestions is that both bring breaking changes, which I try to avoid unless there is a larger change to the API.

Maybe the right solution would be to allow the first argument to be "noPromise". i.e.:

entry.autodrain = function(noPromise) {
  __autodraining = true;
  var draining = entry.pipe(NoopStream();
  if (noPromise) return draining;
  draining.promise = function() {
    draining.on('finish',resolve);
    draining.on('error',reject);
  };
  return draining;
}

No argument is currently defined on .autodrain so this would not technically be a breaking change, the error is not automatically swallowed and you can bypass the empty promise if you want.

What do you think?

[yocontra]

Is it documented anywhere that this returns a promise? I’m open to either, i believe in pulling bandaids but up to you if you think it’s worthy of a major bump.

[ZJONSSON]

My last suggestion would not require a bump, and would allow you the same interface as you suggested, except you would have to entry.autodrain(true).on('error', handler) would be promiseless

[yocontra]

@ZJONSSON Yeah, that works

[ZJONSSON]

Hey @contra I looked more closely at this and I agree with you, i.e. as the returned promise is neither in documents nor any of the tests and examples we can just make the switch.

Did a PR but some of test that rely on external urls are failing. Probably unrelated, will look into that separately.

[ZJONSSON]

published as + unzipper@0.9.3