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.

Sign up for GitHub

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

chore: Use error filter in rn event source #322

Merged
merged 5 commits into from
Dec 2, 2023
Merged

chore: Use error filter in rn event source #322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b049c49
chore: use console.log as custom logger to remove annoying error mess…
yusinto Dec 1, 2023
d228357
fix: remove initial connection delay.
yusinto Dec 1, 2023
cd075af
chore: implement custom es retry logic using errorFilter.
yusinto Dec 1, 2023
a6845fc
fix: missing retry argument
yusinto Dec 1, 2023
0c521e0
chore: turn off es debugging.
yusinto Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/sdk/react-native/src/ReactNativeLDClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {
base64UrlEncode,
BasicLogger,
LDClientImpl,
type LDContext,
type LDOptions,
Expand All @@ -9,7 +10,14 @@ import platform from './platform';

export default class ReactNativeLDClient extends LDClientImpl {
constructor(sdkKey: string, options: LDOptions = {}) {
super(sdkKey, platform, options);
const logger =
options.logger ??
new BasicLogger({
level: 'info',
// eslint-disable-next-line no-console
destination: console.log,
});
super(sdkKey, platform, { ...options, logger });
Comment on lines +13 to +20
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use console.log instead of error to remove annoying error messages on the simulator during development.

Copy link
Member

Choose a reason for hiding this comment

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

So, we may want to make a different client-side basic logger. One that just uses the levels of the error.

Being as, in browser, we generally have these levels and they are more meaningful than stdout,stderr.

Doesn't need to be in this PR though.

logger: {
      debug: (...args) => {
        console.debug(...args);
      },
      info: (...args) => {
        console.info(...args);
      },
      warn: (...args) => {
        console.warn(...args);
      },
      error: (...args) => {
        console.error(...args);
      },
    },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a ticket to capture this for the future.

}

override createStreamUriPath(context: LDContext) {
Expand Down
6 changes: 4 additions & 2 deletions packages/sdk/react-native/src/platform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ import RNEventSource from './react-native-sse';

class PlatformRequests implements Requests {
createEventSource(url: string, eventSourceInitDict: EventSourceInitDict): EventSource {
// TODO: add retry logic
return new RNEventSource<EventName>(url, eventSourceInitDict);
return new RNEventSource<EventName>(url, {
headers: eventSourceInitDict.headers,
retryAndHandleError: eventSourceInitDict.errorFilter,
});
}

fetch(url: string, options?: Options): Promise<Response> {
Expand Down
25 changes: 21 additions & 4 deletions packages/sdk/react-native/src/react-native-sse/EventSource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ const defaultOptions: EventSourceOptions = {
method: 'GET',
pollingInterval: 5000,
timeout: 0,
timeoutBeforeConnection: 500,
timeoutBeforeConnection: 0,
withCredentials: false,
retryAndHandleError: undefined,
Comment on lines +20 to +22
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I settimeoutBeforeConnection to 0 to get flags asap on initial connection.

};

export default class EventSource<E extends string = never> {
Expand Down Expand Up @@ -49,6 +50,7 @@ export default class EventSource<E extends string = never> {
private xhr: XMLHttpRequest = new XMLHttpRequest();
private pollTimer: any;
private pollingInterval: number;
private retryAndHandleError?: (err: any) => boolean;

constructor(url: string, options?: EventSourceOptions) {
const opts = {
Expand All @@ -65,6 +67,7 @@ export default class EventSource<E extends string = never> {
this.body = opts.body;
this.debug = opts.debug!;
this.pollingInterval = opts.pollingInterval!;
this.retryAndHandleError = opts.retryAndHandleError;

this.pollAgain(this.timeoutBeforeConnection, true);
}
Expand Down Expand Up @@ -147,7 +150,21 @@ export default class EventSource<E extends string = never> {

if (this.xhr.readyState === XMLHttpRequest.DONE) {
this.logDebug('[EventSource][onreadystatechange][ERROR] Response status error.');
this.pollAgain(this.pollingInterval, false);

if (!this.retryAndHandleError) {
// default implementation
this.pollAgain(this.pollingInterval, false);
} else {
// custom retry logic
const shouldRetry = this.retryAndHandleError({
status: this.xhr.status,
message: this.xhr.responseText,
});

if (shouldRetry) {
this.pollAgain(this.pollingInterval, true);
}
}
}
}
};
Expand Down Expand Up @@ -290,7 +307,7 @@ export default class EventSource<E extends string = never> {
this.onerror(data);
break;
case 'retry':
this.onretrying();
this.onretrying({ delayMillis: this.pollingInterval });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bugfix. The StreamingProcessor expects this argument.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so in the future we could add jitter/backoff here. By calculating it and passing it as the delayMillis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I also created a ticket for this work.

break;
default:
break;
Expand All @@ -310,5 +327,5 @@ export default class EventSource<E extends string = never> {
onopen() {}
onclose() {}
onerror(_err: any) {}
onretrying() {}
onretrying(_e: any) {}
}
1 change: 1 addition & 0 deletions packages/sdk/react-native/src/react-native-sse/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export interface EventSourceOptions {
body?: any;
debug?: boolean;
pollingInterval?: number;
retryAndHandleError?: (err: any) => boolean;
}

type BuiltInEventMap = {
Expand Down