-
Notifications
You must be signed in to change notification settings - Fork 127
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
Support for server-sent events (EventSource) #99
Comments
The main question I always have for proposals like this is why does this feature need to be in core vs a 3rd party library? |
(for reference: #83) |
I see in the other discussion referenced that @cpojer says that the "current focus is to reduce the surface area instead of adding to it." I understand this concern when it comes to non-standard libraries or features that don't exist by default in other JavaScript environments. Redux for instance is an example of a library I would completely agree should remain as a 3rd party library and be out of core. However, React Native includes the I'd like to hear from @TheSavior and @cpojer on why they believe In my opinion, when JavaScript environments like React Native choose to exclude standard features like this, it further fragments the already messy JavaScript ecosystem, making it a lot more difficult to build universal JavaScript libraries that work across platforms. I work on a third-party library that intends to support React Native among other platforms (MongoDB Stitch JS SDK), but we've run into many issues along the way with platform-specific bugs and feature gaps, and we've had no choice but to package React Native-specific, Node.js specific, and browser-specific libraries that sit on top of a platform-independent JS codebase that abstracts away features that aren't natively supported by all those environments. Fortunately we have the time and resources to do that, but many independent library developers do not. If React Native doesn't intend to support features like |
Hello @adamchel, thank you for bringing up this issue. I think @TheSavior's question is a good one, and as you pointed out yourself we are actively trying to reduce the surface area of React Native. In terms of which standards we intent to support, I think it is too early in the lifetime of React Native to settle on any one standard. However, keep in mind that we are not attempting to build a standards compliant web browser, so we will likely never match features in modern browsers exactly. Absent of having a standardization process or spec to follow, I think we should include features that the majority of people will find useful. This is also why I do think the best course of action is to either go with Even if we were to consider including this into core at some point in time, it would most likely be an additional plugin that people would have to enable. This is why I'd prefer to help improve the ecosystem around React Native – if some extensions get significant traction, we should consider shipping them by default. |
Thanks for your input. I really appreciate the quick responses on this. I totally understand that you don't want to clutter the core React Native library with things that most people won't find useful. I'll offer two more counterpoints but I'm getting the sense that you'd prefer we use and improve a third-party library before you'll consider including this in core 😃. As for your point on
Server-sent events are more more widely used than you might think. The most popular eventsource polyfill has 4.9M downloads on NPM. Meanwhile, the most popular libraries for My second point is that as far as I know, there is no native mechanism for simple HTTP streaming in React Native, whether it's via Are there plans to implement the |
In general I think there are three approaches we could take: Add this functionality to core The problem with adding this functionality to core is that we aren’t the experts on this behavior. You really don’t want us to be the gatekeepers for this code. It would move too slowly and not get the attention it deserves. I think one thing you are really asking for in this issue is for Facebook to step in and create a high quality eventstream library because the existing 3rd party ones aren’t good enough. I think it is way less likely that we’d create a good one in core than the community could band together and create a good one themselves from the people who care most about this behavior. The second approach, linking a well supported 3rd party library by default is more interesting as it would enable the community to iterate freely and then a compatible library would be linked. However, the major downside to this is that it bloats React Native for everyone, including those who don’t use it. We want to decrease the size required by React Native apps and the way to do that is to enable apps to easily add what they need. This leaves us with the third approach, the one we have today. Use a 3rd party library in your app to add the functionality you need. I think this is likely where we’ll stay on this due to the reasons outlined for the other approaches. |
@TheSavior thanks, that makes a lot of sense. My last question then is, what about implementing the Streams API for Network streaming isn't something that you can just easily implement with a third-party library on top of the network primitives that exist in React Native today. You need more low-level access to the networking, which the existing |
Agreed with @adamchel, if you don't think it's appropriate for you to provide native/bundled If you won't implement the standard, please expose the primitives needed for others to do so. |
I’m not sure what is involved in making that happen but that makes some sense to me. |
@TheSavior would you prefer if we opened a new proposal in this repo for supporting a low-level networking API and/or the And I also just wanted to clarify why we believe XHR is not sufficient. XHR doesn't let you clear the incoming streamed response buffer as data comes in (more context here). This means that as data comes in and your application processes it, it can't clear the buffer without starting a new XHR, which defeats the purpose of streaming data. This has implications for mobile devices, where if you have a long-running stream of data, memory use is going to shoot up unless you restart the stream. The |
As for what needs to be done to make this happen, I'm not too familiar with the React Native source code, but from some light perusing, it seems like the low-level Interestingly, you implement The low-level
If Facebook were able to expose a simple request API using |
Thanks for the investigation! I’d recommend repurposing this discussion. I think we are probably open to the idea of exposing some hooks to RCTNetworking but I wouldn’t expect us to have any bandwidth to make that change. The best way to probably make this happen is to investigate the smallest technical changes to core necessary to enable what you need to happen in user space and make sure that is of reasonable scope to be worth sending a PR or if we need to expand the discussion first. Does that make sense and seem reasonable? |
I think that sounds reasonable. When we have the bandwidth (probably sometime in the next few weeks), I can work on producing a PR to create the necessary hooks. As far as scope goes, I'm imagining two possible approaches:
Whatever we end up producing, whether it is I personally would prefer the second approach since it's more standard, but would love to hear your thoughts. We wouldn't want to start any work on this unless we know you'd be open to accepting a PR with the approach we go with. |
After looking into implementing full-blown Given the audience for RN and its platform footprint, I think that Thank you for taking this up! |
On the Android side, we should also see if we can use this as a way to be able to switch out OKHTTP. |
Thanks for this productive discussion. I think we have reached a consensus that React Native's native code needs to change to support this, and we are open to receiving PRs for that. I will close this issue here given that we agree, but feel free to continue the discussion here until a PR is sent. As @axemclion is pointing out, ideally this will also come with Android support. I am fine with the second option, even if the first option would be ideal for long term support. |
Summary: This PR introduces the `EventSource` web standard as a first-class networking feature in React Native. In the discussion we had in February at react-native-community/discussions-and-proposals#99, cpojer indicated that the RN maintainers would be willing to accept a PR to offer this functionality. The linked discussion goes into detail about why this change must happen in React Native Core as opposed to a community library, but the tl;dr is that `XmlHttpRequest` doesn't let you do streaming in a resource-efficient way, since it holds onto the entire response buffer until the request is complete. When processing a stream that might last for a long time, that's not ideal since there might be a lot of data in that buffer that is now useless to maintain. For more information about EventSource and server-sent events, check out these links: * [EventSource on MDN](https://developer.mozilla.org/en-US/docs/Web/API/EventSource) * [Using server-sent events on MDN](https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events) * [WHATWG spec for server-sent events](https://html.spec.whatwg.org/multipage/server-sent-events.html) I've tried as best as I can to satisfy the linked specification so that this is as standard as possible. One of the projects I maintain has an ideal use case for this feature. The SDK for MongoDB Stitch (a backend-as-a-service for the MongoDB database) has the ability to open a "change stream" to watch for changes that happen on a database. However, in our JavaScript SDK, this feature depends on `EventSource`, because the backend service implements the one-way streaming protocol with server-sent events. We know there is demand for this feature because users have requested it: mongodb/stitch-js-sdk#209. If this PR will be accepted, I am happy to update the `Networking` documentation at https://facebook.github.io/react-native/docs/network ## Changelog [JavaScript] [Added] Implements the `EventSource` web standard in `Libraries/Networking` [JavaScript] [Added] Exposes the `EventSource` implementation in `Libraries/Core/setUpXHR.js` Pull Request resolved: #25718 Test Plan: To test the `EventSource` implementation, I added a comprehensive set of unit tests that cover the basic functionality, as well as edge cases that are laid out in the spec. See `EventSource-test.js` for the cases that the tests handles. For convenience, I've also included the test descriptions as produced by the `jest` test output here. ``` PASS Libraries/Network/__tests__/EventSource-test.js EventSource ✓ should pass along the correct request parameters (527ms) ✓ should transition readyState correctly for successful requests (4ms) ✓ should call onerror function when server responds with an HTTP error (2ms) ✓ should call onerror on non event-stream responses (1ms) ✓ should call onerror function when request times out (1ms) ✓ should call onerror if connection cannot be established (1ms) ✓ should call onopen function when stream is opened (1ms) ✓ should follow HTTP redirects (2ms) ✓ should call onmessage when receiving an unnamed event (2ms) ✓ should handle events with multiple lines of data (1ms) ✓ should call appropriate handler when receiving a named event (1ms) ✓ should receive multiple events (1ms) ✓ should handle messages sent in separate chunks (1ms) ✓ should forward server-sent errors ✓ should ignore comment lines (1ms) ✓ should properly set lastEventId based on server message (1ms) ✓ should properly set reconnect interval based on server message ✓ should handle messages with non-ASCII characters (1ms) ✓ should properly pass along withCredentials option (3ms) ✓ should properly pass along extra headers (1ms) ✓ should properly pass along configured lastEventId (2ms) ✓ should reconnect gracefully and properly pass lastEventId (9ms) ✓ should stop attempting to reconnect after five failed attempts (2ms) ``` As a manual E2E test, I also added streaming support to the Stitch React Native SDK, and tested it with my React Native EventSource implementation, and confirmed that our `EventSource`-based streaming implementation worked with this `EventSource` implementation. * Source code for E2E app test: https://gist.github.com/adamchel/6db456c1a851ed7dd20b54f6db3a6759 * PR for streaming support on our React Native SDK: mongodb/stitch-js-sdk#294 * Very brief video demonstrating E2E functionality: https://youtu.be/-OoIpkAxmcw Differential Revision: D17283890 Pulled By: cpojer fbshipit-source-id: 0e9e079bdb2d795dd0b6fa8a9a9fa1e840245a51
Update: RCTNetwork is will be exposed from react-native from 0.62 (next next release) onwards. See facebook/react-native#25718 Thanks @adamchel |
@adamchel Thanks for driving this. We're using react-native-event-source and would be keen to switch. Happy to help test. |
Hey everyone! I've just published a package that exposes https://www.npmjs.com/package/rn-eventsource I tested it with |
I'm having trouble getting this to work at all on my Android device and on my Android emulator. The server does receive the GET request, but the client never receives any messages. I'm not sure how to troubleshoot or debug this at all. |
It works on ios and ios emulator, it works in Chrome App on ios and Android. I tried using mitmweb but that didn't show the returned Events. In the Android Studio Network Traffic Monitoring tool, the returned events do not show up.
Here are the differences in the headers sent in the request: Android iphone: } |
@kovkev, I don't think this is the right location for your issue as this was a proposal posted over a year ago and finished. If you are having an issue with RCTNetworking then opening an issue in the main repo would be the right choice. If you are having issues with eventsource, opening an issue in https://github.com/adamchel/rn-eventsource is probably a good choice. |
I'm not sure if the issue is with RCTNetworking or with rn-eventsource. My guess is that since this is failing with both rn-eventsource and with simple |
Introduction
Server-sent events are a standard W3C-specified and WHATWG-specified feature that supports processing real-time events from a server over an HTTP text streaming protocol.
All modern browsers (except Edge) have supported this feature for years, and for Edge it is a feature request with 6.7k votes.
There is no support in React Native, nor could I find any open issues for it. I believe React Native should have built-in support these types of streaming requests just like it supports
fetch
andWebSockets
.The Core of It
The introduction covers the gist of the issue and the proposal, but I will also point that while there are existing polyfills for this feature, they are not sufficient, and I think built-in support is the best solution. Below, I've outlined why I think the existing polyfills are insufficient.
Existing Polyfill 1 : eventsource
With 4.9M weekly downloads, this is the definitive EventSource polyfill and it works well on browsers and Node.js, but unfortunately it uses the node standard library, which is not supported by React Native.
Existing Polyfill 2: react-native-event-source
This library specifically supports React Native with its EventSource implementation, but it is based on a polyfill that wraps
XmlHttpRequest
, which is not optimal for network streaming.The polyfill does work, but it doesn't have much usage, and it doesn't play nicely with the default
jest
configuration for react native (see jordanbyron/react-native-event-source#14). I shouldn't have to run through a lot of hoops to test code using such a basic networking feature.Existing Polyfill 3: react-native-eventsource
A new version of this polyfill hasn't been released in two years, it appears to have only 2 weekly NPM downloads, and it uses native modules, which makes it infeasible to use in simple Expo-based applications.
Discussion points
fetch
andWebSockets
. See Networking in the React Native docs.The text was updated successfully, but these errors were encountered: