chore: Add react-native EventSource #318
Conversation
yusinto
requested review from
kinyoklion and
louis-launchdarkly
as code owners
|
This pull request has been linked to Shortcut Story #223903: Add RN EventSource. |
packages/sdk/react-native/src/polyfills/react-native-sse/LICENSE
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,309 @@ | |||
| /** | |||
There was a problem hiding this comment.
Could you comment on the parts in here that had to change? Aside from being converted to typescript.
There was a problem hiding this comment.
Is the idea we would change this to work with our retry model?
One thing I have disliked in our implementation is the retry logic inside the event source. This event source supports disabling retries, so I was curious if we could just do that and use it without modifications. Then have a small layer that just adapts and adds retry/backoff.
This is effectively what we were doing in client side JS, because the browser doesn't support jitter/backoff.
It doesn't have the benefit of being able to disable retry/backoff, so it does it by closing it.
https://github.com/launchdarkly/js-sdk-common/blob/main/src/Stream.js
If you made a WithJitterBackoff implementation, then you could probably use it for react-native and browser.
There was a problem hiding this comment.
Right now due to time constraints, to use whatever retry logic that is originally in the repo without changes. I am trying to get an alpha release that supports streaming in the most rudimentary fashion again in the interest of time.
There was a problem hiding this comment.
Could you comment on the parts in here that had to change?
The idea is to own this code from now and not be concerned with syncing our copy with the original copy. I'm hesitant to maintain a list of changes every time we make changes to this code. I can record what I changed in this PR so we have a record, but to maintain a list every time is unnecessary and burdensome imho.
There was a problem hiding this comment.
Being as we didn't start with the base version I don't have any focus to review.
There was a problem hiding this comment.
Ok no worries. I added major changes as comments per your request.
|
|
||
| import { name, version } from '../package.json'; | ||
| import { btoa, uuidv4 } from './polyfills'; | ||
| import { btoa, EventSource as RNEventSource, uuidv4 } from './polyfills'; |
There was a problem hiding this comment.
I am a little hesitant to call it a polyfill.
There was a problem hiding this comment.
I'm not too fussed and will adopt a better term/convention apart from polyfills. Any suggestions?
There was a problem hiding this comment.
If you were just writing some code to stream events, and it wasn't some feature browsers happened to have, then what would you call it?
I would probably have something like network/event-source or something. I am not too critical right now, but polyfill has implications that I would rather not have longer term.
There was a problem hiding this comment.
I moved the react-native-sse folder out of polyfills so its directly under src for now. Is this ok?
This PR makes minor improvements to the StreamingProcessor.
This adds a local customised copy of react-native-sse. I have modified this to be in TypeScript and added minimal extra logic to work with our use case.