STITCH-2499 Add support for 'watch' in React Native #294
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Sep 11, 2019
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
|
Hello everybody! I'm just a Junior Developer and just want to understand what's going on right now. React Native added support for the proper networking components to work? |
|
This won't be supported until React Native 0.62 is released. |
tkaye407
reviewed
Jan 31, 2020
| respHeaders[key] = value; | ||
| }); | ||
| if (response.status < 200 || response.status >= 300) { | ||
| return response.text() |
There was a problem hiding this comment.
Does response.text() return a promise?
There was a problem hiding this comment.
from lib.dom.d.ts
interface Body {
readonly body: ReadableStream<Uint8Array> | null;
readonly bodyUsed: boolean;
arrayBuffer(): Promise<ArrayBuffer>;
blob(): Promise<Blob>;
formData(): Promise<FormData>;
json(): Promise<any>;
text(): Promise<string>;
}
tkaye407
approved these changes
Jan 31, 2020
There was a problem hiding this comment.
LGTM. There is a case to be made that you should split up your tests a bit more as they are not very isolated, but if you disagree let me know. Also, I would probably argue to add in some kind of handler to wrap the stream, but I guess the new SDK's can do this
| @@ -749,4 +753,262 @@ describe("RemoteMongoClient", () => { | |||
|
|
|||
| expect(coll.findOne()).rejects.toBeTruthy(); | |||
| }); | |||
|
|
|||
| /* tslint:disable:no-string-literal */ | |||
| it("should properly support watch streams on set of ids", async () => { | |||
There was a problem hiding this comment.
I think you should split this up into smaller tests personally. This is essentially just 4 tests put into one test
There was a problem hiding this comment.
Discussed offline, these tests are basically just copy-pasted from the watch tests in the other two SDKs. We're going to leave as is.
jsflax
approved these changes
Jan 31, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds support for "watch" functionality in React Native, as requested by #209.
However, this change depends on facebook/react-native#25718 getting accepted and released in a future release of React Native:
I will be keeping this PR open until that happens.