writable streams prevent async-done from detecting completion

[RoystonS]

(I'm assuming that the intent of async-done, when given a stream, is to detect when the stream has finished emitting data. If that's not true, this bug report is invalid.)

I've been trying to diagnose a curious problem with a combination of merged streams (using the event-stream module) and have come across a case where async-done doesn't detect that a stream has finished emitting data.

If the stream reports itself as writable, the end-of-stream module, by default, waits for both the readable side to complete (via an 'end' or 'close' event) and for the writable side to complete (via a 'finish' event). The read/write streams returned by event-stream#merge don't ever complete their write side, so async-done never detects that the stream has finished (emitting).

Changing the EOS configuration in async-done to include 'writable': false tells end-of-stream that we don't care whether the stream is still writable, i.e.

var eosConfig = {
  error: false,
  writable: false
};

[phated]

Would you be able to submit a failing test? I'd like to poke @chrisdickinson about this because I though stream-exhaust was supposed to handle this case.

[chrisdickinson]

@phated stream-exhaust only exhausts the stream (i.e., reliably puts it into a "flowing" state). event-stream is simulating the end event propagation, which seems to confuse end-of-stream.

[phated]

@chrisdickinson is the best course of action to de-support event-stream? I've always found it to be lackluster or even bad at times. I believe everything it does has a corresponding streams2 implementation on npm. Thoughts?

[RoystonS]

stream-exhaust and event-stream don't seem to play nicely together because the stream returnbed by event-stream#merge doesn't actually contain a 'resume' function. We have our own extended version of event-stream which adds (no-op) pause and resume functions. I think that still satisfies the contract? But it does produce a stream that doesn't get detected as complete by async-done because async-done tells end-of-stream (by not specifying the 'writable: false' flag) that it cares about the write side of the stream as well as the read side.

I've hacked around this by modifying our patched stream so that it emits 'finish' just after it emits 'end', but that's clearly a rubbish hack.

Whilst event-stream is obviously providing a bit of a dopey stream implementation, it still seems that async-done shouldn't be caring about whether the stream is writeable or not, should it? That's why I was thinking that providing the 'writable: false' parameter to end-of-stream may be the right thing to do.

I'll happily provide a failing test, but it's only worth my doing that if the intent is that async-done shouldn't care about whether the stream remains writable or not. What's is the contract?

[phated]

I'm really leaning toward de-supporting the event-stream module in gulp4 - @contra @sindresorhus @terinjokes thoughts?

[yocontra]

@phated We should probably look how many gulp plugins are using it

[yocontra]

From a quick look there are 418 public plugins using it, which is ~1/3rd of all public plugins. I think we can desupport it if we have enough time to go issue 418 pull requests which could be automated semi-easily

[phated]

Major bump, breaking changes, tell them to switch to through2 like they were supposed to already do. That being said, I think the main problem is returning an event-stream from the task, not piping them together.

[yocontra]

@phated A major bump to gulp means nothing for the plugin ecosystem since these are used by other systems, not just gulp. The spec the plugin authors wrote against hasn't changed in 4.0, this is just a bug in event-stream. No need to overreact and break things hastily.

[phated]

Let me rephrase, de-support event-stream pseudo-streams from being returned from a task. Plugins based on event-stream won't be affected because we start/end (not sure which one takes effect) with a real node stream.

[phated]

event-stream#merge can be replaced with https://www.npmjs.com/package/merge2

[phated]

ref https://github.com/chrisdickinson/stream-exhaust/pull/2

[phated]

Should no longer need the custom noop resume, but we won't sink the stream. I suggest switching to merge2

[yocontra]

@phated What do you think about taking the list of modules tagged with gulpplugin that have event-stream as a dependency and opening an issue to switch off of it with a list of the modules they should move to?

[phated]

@contra I think that'd a good strategy. We can also suggest switching out gulp-util at the same time.

[yocontra]

@phated Yeah, pre gulp 4 we should have something to automate those issues being opened on repos that depend on gulp-util and any other modules being deprecated

[phated]

Added note about event-stream being unsupported. Still need to find a good way to reach out to other modules about this.