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

Simpler event stream sub #278

Merged
merged 5 commits into from
Apr 8, 2017
Merged

Simpler event stream sub #278

merged 5 commits into from
Apr 8, 2017

Conversation

calvinmetcalf
Copy link
Contributor

fix for #277, hopfully

@mcollina
Copy link
Member

mcollina commented Apr 7, 2017

LGTM and let's see what happens.

@calvinmetcalf
Copy link
Contributor Author

wait actually I have a better idea

@calvinmetcalf
Copy link
Contributor Author

we already are successfully using the package.browser switch, so lets use it for this

@calvinmetcalf
Copy link
Contributor Author

ok @mcollina check this out

@RangerMauve
Copy link
Contributor

@calvinmetcalf You'll want to apply these changes to the react-native field, too, or get rid of it to use the browser version instead.

@calvinmetcalf
Copy link
Contributor Author

ok good call

@RangerMauve
Copy link
Contributor

RangerMauve commented Apr 7, 2017

Err, wait, it'll still break on loading the stream module. Maybe copy the whole browser object, and for React-Native add "stream": false. Since it needs all the changes for browser compatibility, plus it needs the stream module to be explicitly excluded, else the require("st" + "ream") call will crash the bundler.

@calvinmetcalf
Copy link
Contributor Author

there shouldn't be any stream calls anymore in the browser code

@RangerMauve
Copy link
Contributor

You're right, sorry about that. :P

@calvinmetcalf
Copy link
Contributor Author

so it's good ?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can pull https://github.com/nodejs/node/blob/master/lib/internal/streams/legacy.js from core. That's what there is in stream.

@calvinmetcalf
Copy link
Contributor Author

@mcollina the idea isn't that we need the same api, it's that instanceof stream checks should succeed

@mcollina
Copy link
Member

mcollina commented Apr 7, 2017

LGTM

@mcollina mcollina merged commit 48086bc into master Apr 8, 2017
@mcollina mcollina deleted the simpler-event-stream-sub branch April 8, 2017 08:04
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

Successfully merging this pull request may close these issues.

None yet

3 participants