chore (Hooks): Move common client ready logic to a separate function #34
Conversation
src/hooks.ts
Outdated
| const [data, setData] = useState<UseFeatureState>(() => { | ||
| if (isServerSide) { | ||
| return getCurrentValues(); | ||
| } | ||
| return { isEnabled: false, variables: {} }; | ||
| }); | ||
|
|
||
| const [clientReady, setClientReady] = useState(isServerSide ? true : false); | ||
| const [didTimeout, setDidTimeout] = useState(false); |
There was a problem hiding this comment.
To reduce the number of arguments going around, is it possible to collapse all this into one useState call returning an object with all these things? Then set different parts of it as needed?
There was a problem hiding this comment.
I initially thought this would be a hassle since the useState set state method doesnt extend by default, making it necessary to return the entire state object.
However, I think you're right that it's worth it here, and it makes the typing simpler too.
Set state calls now look like this:
setState((state: HookState) => Object.assign({}, state, { didTimeout: true }));
|
@mjc1283 @circAssimilate @pauloptimizely - Gonna merge this now to proceed but happy to address any feedback in follow up PRs! |
Summary
In preparation to introduce a
useExperimenthook, this PR moves the logic for waiting for the client to become ready, setting initial state, and configuring autoupdate to a shared function, which is passed touseEffectThere are a lot of variables that need to be passed, but I think this is much preferable to duplicating the somewhat intricate & nuanced logic that exists there.
Test Plan
Existing unit tests cover this functionality, and more will be added with the introduction of
useExperiment