-
Notifications
You must be signed in to change notification settings - Fork 43
fix: surface get flags initialization errors #323
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
base: main
Are you sure you want to change the base?
Conversation
flagsmith-core.ts
Outdated
@@ -498,22 +504,33 @@ const Flagsmith = class { | |||
} | |||
if (shouldFetchFlags) { | |||
// We want to resolve init since we have cached flags | |||
|
|||
this.getFlags().catch((error) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kyle-ssg , I'm a bit unconfortable here due to this test:
test('should not reject but call onError, when the identities/ API cannot be reached with the cache populated'
Just to give you context. This PR is really specific to flagsmithOnFlagsmith, hence why you can find a check flagsmith !== defaultApi
.
I don't want to change the generic SDK behavior.
However here it feels like there is some redundancy => Is there a reason why we don't check shouldFetchFlags
a bit earlier and have if (cachePopulated && !shouldFetchFlags)
? A fallback reason ? Maybe we could deal with it in case of failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it would help me to understand what's the behavior we want in case of failure. While working on this, there are a couple of points that are unclear to me, especially concerning an API error.
Why don't we want to throw the errors directly and stop the initialization?
To me the cases we have are:
- Default flags + API error => Initialize with default flags, trigger onError
- Valid Cache + API error => Initialize with cache, trigger onError
- API error => Retries and throw error
Well, it's a bit particular with FoF as for most of the flags we are dealing with the platform ones, that shouldn't be critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the code looks good but I'd be more confident if we first confirm the expecte error handling
await this.getFlags(); | ||
} catch (e) { | ||
this.log('Exception fetching flags', e); | ||
if (this.api !== defaultAPI) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not work as expected with FoF
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
Related to #320
Throw early error if
environmentID
orapi
are not set. Another issue is that with wrong configuration, the initialization would fail silently while havinginitialised = true
, leading to extra requests failing and causing errors.The goal of this PR was to surface the initial
getFlags
error in order to be able to handle an initialisation error.How did you test this code?
Steps to setup: