-
Notifications
You must be signed in to change notification settings - Fork 48
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: ensure we don't send multiple flag requests in parallel #423
base: main
Are you sure you want to change the base?
Conversation
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.
PR Summary
This PR implements request deduplication for feature flag fetches to prevent flooding the API with parallel requests when getFeatureFlags() is called multiple times in quick succession.
- Added
inflightRequest
property to track and reuse in-progress feature flag fetch requests - Modified
_loadFeatureFlags()
to check for existing requests before initiating new ones - Added guard in
loadFeatureFlags()
to wait for inflight requests to complete before returning - Properly cleans up
inflightRequest
reference after request completion or failure - Fixes potential rate limiting issues by preventing duplicate API calls during initialization
2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -81,6 +81,7 @@ class FeatureFlagsPoller { | |||
this.projectApiKey = projectApiKey | |||
this.host = host | |||
this.poller = undefined | |||
this.inflightRequest = undefined |
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.
syntax: Missing type declaration for inflightRequest class property
this.inflightRequest = undefined | |
inflightRequest?: Promise<PostHogFetchResponse> | |
this.inflightRequest = undefined |
const res = await this.inflightRequest | ||
this.inflightRequest = undefined |
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.
style: Consider adding error handling around the await to ensure inflightRequest is always cleared, even if the request fails
const res = await this.inflightRequest | |
this.inflightRequest = undefined | |
try { | |
const res = await this.inflightRequest | |
return res | |
} finally { | |
this.inflightRequest = undefined | |
} |
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.
Nice, makes sense and posthog-js does this already
Problem
currently if we call e.g.
getFeatureFlags()
100 times straight away after initialisation we will send 100 fetch requests before the first one returnsthis is even worse if e.g. we are erroring so never setting
loadedSuccessfullyOnce
Changes
add logic to check if we have an inflight request already and block on the existing one instead of starting a new one
Release info Sub-libraries affected
Bump level
Libraries affected
Changelog notes