fix: react context no longer changes on flag change #1081
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
joker23
force-pushed
the
skz/react-native-rendering
branch
4 times, most recently
from
5936ae2 to
baed8fb
Compare
| break; | ||
| } | ||
|
|
||
| if (newValue !== valueRef.current) { |
There was a problem hiding this comment.
Object types will still run into problems since they will not test equal, but I think it is still an improvement from before.
We can look into doing a deeper compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
packages/sdk/react-native/src/hooks/variation/useTypedVariation.ts
Outdated
Show resolved
Hide resolved
joker23
force-pushed
the
skz/react-native-rendering
branch
from
baed8fb to
152a584
Compare
joker23
force-pushed
the
skz/react-native-rendering
branch
from
152a584 to
2086a07
Compare
| return () => { | ||
| ldClient.off('change', handleChange); | ||
| }; | ||
| }, [key]); |
There was a problem hiding this comment.
maybe we would want to reregister if defaultValue changes?
Requirements
Related issues
sdk-1792
Describe the solution you've provided
Describe alternatives you've considered
I think this change may require a major version, but we can probably avoid that with some compromises.
Additional context
There is a testing gap here and I will create some tasks to address. I want to do this in a different PR because I would like to refactor RN unit test to use https://oss.callstack.com/react-native-testing-library/ instead of the react testing library.
We also need to revisit the detox test, from some research it seems like detox is the correct technology for e2e testing, but we may want to revisit how to plug in to the sdk test harness (and deprecate the example testing)
Note
Medium Risk
Changes how flag updates propagate (from context updates to per-hook subscriptions), which can affect rendering behavior and relies on correct event listener management.
Overview
Ensures the
LDProvidercontext value no longer changes on flag updates by removing thechangelistener/state indirection (setupListeners) and memoizing{ client }instead.Moves reactivity to the flag hooks:
useTypedVariationanduseTypedVariationDetailnow keep local state, subscribe to the client’schangeevent, and update only when the evaluated value changes (with properoffcleanup), while still normalizingreasontonullwhen absent.Removes the
setupListenersunit test and updates theLDProvidertest to stop mockingsetupListeners.Written by Cursor Bugbot for commit 2086a07. This will update automatically on new commits. Configure here.