-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Let the user settings hook just use the first defined value #42605
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
a672a24
to
6628f19
Compare
Graphite Automations"Notify author when CI fails" took an action on this PR • (05/17/24)1 teammate was notified to this PR based on Raphael Krut-Landau's automation. "Don't backport" took an action on this PR • (05/17/24)1 label was added and 1 reviewer was added to this PR based on Raphael Krut-Landau's automation. |
|
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 really needs tests. This is essentially a caching problem which is really hard, and tests will help make sure it works as we expect, and clearly document those expectations.
- I'm not sure memoization like this is the right approach:
Here, we say, for the purposes of this hook, ignore all API values other than the first one. I think this is confusing to reason about, especially when multiple components are subscribing to the same setting.
I think what we really want here is aggressively optimistic updates - where we immediately update the value in the redux store and then don't update the redux store when the API request completes. That way we get completely synchronous behavior client side for these user settings, and hopefully save them server side, but it's no big deal if they don't. This also lets us avoid the weird back/forth race condition possibilities. I also think this should be the default behavior, maybe with the option to turn it off. WDYT?
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.
I'd like to understand a bit more why this was needed. If useSetting is slow, we should fix it for everyone instead of opting in. If you have a singular use case for this, I'd opt for a more composable solution like:
const CheapToRerender = () => {
const [setting] = useSetting('my-setting')
const staleSetting = useFirstDefinedValue(setting);
return (
<ExpensiveToRerender thing={setting.attr} />
)
}
const memoizedValue = useMemo( | ||
() => currentValue, | ||
// eslint-disable-next-line react-hooks/exhaustive-deps -- Update only when currentValue first becomes defined | ||
[currentValue === 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.
I'm not sure this is what memoization is. If you provide a new key
input (line 16) you'll get the previous key's output still. To me this is something like shouldUseFirstValue
or shouldUseStaleValue
.
return [currentValue, shouldDebounce ? debouncedSetter : setter]; | ||
return [ | ||
shouldMemoize ? memoizedValue : currentValue, | ||
shouldDebounce ? debouncedSetter : setter, |
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.
If this were going to go through with this, I think there should be a setter that does something. You should be able to at the very least have your value update to the setter's value.
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.
Not sure I follow
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.
const [value, set] = useSetting('thing', { shouldMemoize: true });
console.log(value) // 'a'
// somewhere else
set('b');
// next render
const [value, set] = useSetting('thing', { shouldMemoize: true });
console.log(value) // 'a' <-- still a even though we set it to b
I've changed the prop to |
The prop makes it possible to remove a race condition in the Browse models filter: Avoid race condition in Browse models filter. The race condition could arise for any similar UI that tracks a value in the API, so a general solution seemed useful |
Agreed. Will write some tests.
I like this idea! |
Closing for now. Will return to this if the priority is determined to be higher. |
This pull request adds a new option to to the
useUserSetting
hook: just use the first defined value. This helps avoid unnecessary re-renders, which avoids race conditions.In the
useUserSetting
function, an additional parametershouldUseFirstDefinedValue
was added in this branch. If this parameter is true, only the first value retrieved from the API is used. This parameter is optional and set to false by default.