Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The streams produced by the plugin should emit the "end" event rather than "finish". #540

Closed
lddubeau opened this issue Sep 8, 2017 · 3 comments
Labels

Comments

@lddubeau
Copy link
Contributor

lddubeau commented Sep 8, 2017

Expected behavior:

The streams produced by the plugin should emit the end event when there is no more data to be read.

Actual behavior:

The streams emit the finish event.

Discussion:

The streams that the plugin produce are Readable streams. The Node documentation does not define a finish event for Readable streams. It is defined for Writable streams (and anything that implements the Writable API, like Duplex.).

The analogue of finish for Readable streams is the end event, which unfortunately the plugin does not emit.

finish was implemented in response to #245.

I'm not just submitting a PR right off the bat because I think this needs discussion before implementation. For one thing, dropping finish immediately would break existing code, like for instance the code people have given as workarounds in #295.

@ivogabe ivogabe added the Bug label Sep 20, 2017
@ivogabe
Copy link
Owner

ivogabe commented Sep 20, 2017

I thought that gulp-typescript previously would emit both events, but that probably broke in some new version of gulp-typescript (or Node?). Anyways, the generated stream is a Duplex stream so it should emit both events and we don't need to break users by removing the finish event.

Also, the stream has two fields, js and dts, which are readable streams. I'm afraid that they also might not fire the end-event.

Feel free to send a PR!

@lddubeau
Copy link
Contributor Author

I was not precise enough in my description.

When I look at the code I see the "full" stream is a CompileStream and indeed Duplex. However, the .js and .dts streams are CompileOutputStream and only Readable. Yet, all the streams emit finish:

		this.streamFull.emit('finish');
		this.streamJs.emit('finish');
		this.streamDts.emit('finish');

Neither streamJs nor streamDts should emit finish, as they are Readable but not Writable or Duplex.

Is there something I'm missing?

I'm unlikely to produce a PR for this in the short term, since getting this cleaned up is not a pressing matter for me. I've worked around it, and there are other issues elsewhere in the TS ecosystem that are more pressing to me at the moment. I wanted to file an issue though so that the problem is reported in case someone else runs into it.

@ivogabe
Copy link
Owner

ivogabe commented Sep 20, 2017

Ah, I see. That last two lines should then be removed. Though that would be something for the next major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants