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

fix: Handle uncaught fetch exception #256

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

frankieyan
Copy link

Closes #249

When the local cache is populated or when default flags are provided, flagsmith.init does not reject if the call to edge proxy fails; however, an uncaught exception gets thrown which is difficult to catch in user code. This change allows the error to be caught within Flagsmith (logging Exception fetching cached logs when verbose mode is on).

@frankieyan
Copy link
Author

Hi @matthewelwell, would you be able to take a look at this one please?

@kyle-ssg
Copy link
Member

kyle-ssg commented Oct 9, 2024

Hmm I'm not sure I understand this, if we await the api call this makes the cache useless since awaiting flagsmith.init would block until the API returns a response?

@frankieyan frankieyan force-pushed the fix/handle-uncaught-fetch-exception branch from 7b0f640 to db9e757 Compare October 9, 2024 08:01
@frankieyan
Copy link
Author

Hmm I'm not sure I understand this, if we await the api call this makes the cache useless since awaiting flagsmith.init would block until the API returns a response?

That's what I realized just now looking at the failed test. Nice catch! 👍

@kyle-ssg
Copy link
Member

kyle-ssg commented Oct 9, 2024

Hmm I'd be interested in knowing what your ideal solution would be to the issue. Is the purpose of this so you can surface error logs?

The thrown exception will cause onError to call

flagsmith.init({onError:()=>})

@frankieyan
Copy link
Author

frankieyan commented Oct 9, 2024

So the crux of the issue for us is that the thrown error can't be handled outside of Flagsmith. It doesn't reject the promise returned from init, nor does it call onError. This triggers an unhandled exception and turns out is lighting up our Sentry.

As for the error itself, I don't think we're too interested in tracking or handling it as it's likely just due to network flake.

Edit: An alternative could be to do as you suggest: pass onError to getFlags so that it does get called. Is that something you'd prefer?

@kyle-ssg
Copy link
Member

kyle-ssg commented Oct 9, 2024

Yeah I think it should definitely call on error, so that'll need fixing. From what I understood it did since init is all wrapped in a try catch that calls onError

@kyle-ssg
Copy link
Member

kyle-ssg commented Oct 9, 2024

@frankieyan Could you check whether adjusting to the following works for you

   // We want to resolve init since we have cached flags
    this.getFlags().catch((err) => {
     throw err // triggers call to onError
   });

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.

Uncaught rejected promise when attempting to fetch flags while offline
2 participants