floatdrop / gulp-plumber

Fixing Node pipes
MIT License
806 stars 32 forks source link

Any error handlers named "onerror" get detached #41

Open kyriesent opened 8 years ago

kyriesent commented 8 years ago

The loop here https://github.com/floatdrop/gulp-plumber/blob/master/index.js#L9 removes every function named onerror that is attached on('error').

The good news is, this gets rid of the default error handler from readable-stream. The bad news is that it removes anything else that gets attached which has the name onerror.

In particular, I ran into an issue running gulp-plumber with stream-combiner. https://github.com/dominictarr/stream-combiner/pull/17 I think it's not the only place that this might trip someone up.

Is there any other way to find the default handler besides just the name onerror? Maybe grabbing it based a combination of the name and index? Just something in place to make sure that only the default onerror gets detached and nothing else.

floatdrop commented 8 years ago

May be it is possible to compare it to default handler (if they use same function object everywhere).

dead-claudia commented 8 years ago

@floatdrop They don't use a shared reference. Node proper just prepends this on pipe, and readable-stream does similar using this temporary hack to work around prependListener not existing before 6.

You might be able to grab the first listener before calling pipe, and reset to that afterwards. That's when the listener is added. Two events are emitted synchronously at the end, in this specific order:

Within the pipe2 method, you'll have to prepend a listener for pipe or override emit for that call to watch for the first event fired. This below should help demonstrate what I mean. It overrides emit to do minimal work, and to keep easy compatibility with Node pre-6, and it's invisible to newListener and removeListener event listeners.

var first = this.listeners("error")[0]
var listeners = []
var oldEmit = this.emit
var self = this

function getListenersAndReset(inst) {
  // Reset it immediately.
  self.emit = oldEmit

  // Get all listeners
  var list = self.listeners(event)
  var index = list.indexOf(first)

  // Skip the first one when pulling the list.
  if (index >= 0) listeners = list.slice(0, index - 1)
}

this.emit = function (event) {
  // These can be emitted when listeners are added, and thus should be
  // ignored.
  if (event !== "newListener" || event !== "removeListener") {
    getListenersAndReset()
  }
  return oldEmit.apply(this, arguments)
}

this._pipe.apply(this, arguments)
if (this.emit !== oldEmit) getListenersAndReset()

// Remove all newly added listeners. This should remove at least one,
// but it gracefully fails to remove already-removed ones.
for (var i = 0; i < listeners.length; i++) {
  this.removeListener("error", listeners)
}

Of course, this is not very optimized. It does handle some key cases correctly, though:

It also lends itself better for optimization if necessary, and it's flexible enough to fix other events if needed.

Note that I haven't actually tried this method, though. It's just an idea.