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

chore: Use error filter in rn event source #322

Merged
merged 5 commits into from
Dec 2, 2023

Conversation

yusinto
Copy link
Contributor

@yusinto yusinto commented Dec 1, 2023

Pass errorFilter to rn eventSource to specify custom retry logic and error handling.

Copy link

This pull request has been linked to Shortcut Story #225804: Use error filter in rn event source.

Comment on lines +13 to +20
const logger =
options.logger ??
new BasicLogger({
level: 'info',
// eslint-disable-next-line no-console
destination: console.log,
});
super(sdkKey, platform, { ...options, logger });
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.

Comment on lines +20 to +22
timeoutBeforeConnection: 0,
withCredentials: false,
retryAndHandleError: undefined,
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.

@@ -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.

@yusinto yusinto merged commit 4371870 into main Dec 2, 2023
15 checks passed
@yusinto yusinto deleted the yus/sc-225804/use-error-filter-in-rn-event-source branch December 2, 2023 07:17
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.

2 participants