fix(flags): Fire callback when decide errors out #1056
Conversation
|
Size Change: +344 B (0%) Total Size: 852 kB
ℹ️ View Unchanged
|
| } | ||
| // :TRICKY: We want to fire the callback even if the request fails |
There was a problem hiding this comment.
This feels like a hack... Why call receivedFeatureFlags when from what I can see the only thing it needs is this._fireFeatureFlagsCallbacks(errorsLoading) - why not just call that directly?
There was a problem hiding this comment.
I also want all the other stuff that this function does (like setting decideEndpointWasHit) which then warns users when they call getFeatureFlag() that flags didn't load in time.
src/posthog-featureflags.ts
Outdated
| const { flags, flagVariants } = this._prepareFeatureFlagsForCallbacks() | ||
| this.featureFlagEventHandlers.forEach((handler) => handler(flags, flagVariants)) | ||
| this.featureFlagEventHandlers.forEach((handler) => handler(flags, flagVariants, errorsLoading)) |
There was a problem hiding this comment.
I really have a pet peeve for functions that just have a bunch of arguments that can be true or 1 or something.
Feels like this is meta info - why not turn it into an object like context: { errors: true } so that it would be easy to extend further in the future.
There was a problem hiding this comment.
Very reasonable, will do 👍
There was a problem hiding this comment.
Makes sense for the public interface to have a context object, but keeping interals as a param. Don't see any need to convert yet, and easy to refactor when/if we have more params to these internal funcs
|
Merging into yours 👍 |
Changes
Fire callbacks with error param so we don't block clients waiting for flags to load.
Noticed this isn't present on the first Decide response, will add it there too.
...
Checklist