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

Should we use is-stream package? #111

Closed
phated opened this issue Sep 23, 2016 · 6 comments
Closed

Should we use is-stream package? #111

phated opened this issue Sep 23, 2016 · 6 comments
Milestone

Comments

@phated
Copy link
Member

phated commented Sep 23, 2016

We currently use instanceof to check if something is a stream. However, the is-stream module on npm uses stream !== null && typeof stream === 'object' && typeof stream.pipe === 'function'; should we switch to the external module?

cc @contra @sindresorhus

@sindresorhus
Copy link

Not all compatible streams inherit from Stream, and even if they do, instanceof doesn't work across realms (vm module contexts).

@darsain
Copy link
Contributor

darsain commented Sep 23, 2016

Yes, definitely ducktype it. instanceof causes issues in so many places.

@phated
Copy link
Member Author

phated commented Sep 23, 2016

On the other hand, people stick pipe onto things that aren't fully streams compatible (see merge in event-stream). We've had to de-support libraries like event-stream because of that.

I do agree that instanceof is generally bad.

@phated
Copy link
Member Author

phated commented Sep 23, 2016

I'd like to get @contra's thoughts though.

@yocontra
Copy link
Member

Yeah, checking for .pipe is good enough for me - instanceof sucks.

@phated phated modified the milestone: 2.0 Sep 23, 2016
@phated phated closed this as completed in 7159e5d Sep 26, 2016
@phated
Copy link
Member Author

phated commented Sep 26, 2016

Done

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

No branches or pull requests

4 participants