fix: make select.web consistent
#57
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Added another fix to solve #58. |
packages/select/src/select.web.tsx
Outdated
| @@ -225,10 +227,17 @@ const ItemContext = React.createContext<{ | |||
|
|
|||
| const Item = React.forwardRef<ItemRef, ItemProps>( | |||
| ({ asChild, closeOnPress = true, label, value, children, ...props }, ref) => { | |||
| const { labelForValueRef } = useRootContext(); | |||
| labelForValueRef.current[value] = label; | |||
There was a problem hiding this comment.
Unfortunately, the react docs say:
"Do not write or read ref.current during rendering, except for initialization. This makes your component’s behavior unpredictable."
There was a problem hiding this comment.
It's probably best to update it with a useEffect.
React.useEffect(() => {
labelForValueRef.current[value] = label;
}, [value, label]);There was a problem hiding this comment.
That's true, but I wanted to avoid the overhead of useEffect. Since the ref is only used in isolation for the parent-child context system and only read within an event handler, I don't foresee any unpredictability in the behaviour.
There was a problem hiding this comment.
The only possible case where this might cause issues is if the association between value and label unexpectedly changes between concurrent renders. However, we already have the implicit guarantee that value and label should always have a static association.
Maybe, to enforce the static-ness of value and label, and follow the initialization guidelines, we treat this as initialization per key?
const { labelForValueRef } = useRootContext();
if (!(value in labelForValueRef.current)) {
labelForValueRef.current[value] = label;
}There was a problem hiding this comment.
The write-ref-on-render pattern is also used for react-use's useLatest.
There was a problem hiding this comment.
@mrzachnugent I've updated the PR to only set the label for each value once. While it's not initializing the entire ref once, it is effectively only initializing each item once.
For each `<Item>` component, keep track of its `value` to `label`
mapping on the `<Root>` component by passing a `MutableRefObject`
that stores the mapping from `<Root>` to `<Item>` via context.
On render, `<Item>` sets the label for its value. `<Root>` can then call
`onValueChange` with `labelForValueRef.current[val] ?? val`, allowing
feature parity with the native implementation.
This approach works perfectly. Since `Item` must be rendered before it
can be interacted with and emit events, this always ensures that
`labelForValueRef[val]` exists before `onStrValueChange` is called.
Furthermore, since the `value` and `defaultValue` prop requires `{
value: string, label: string }`, the initial render also does not have
any issues.
bryanmylee
force-pushed
the
fix-select-label-consistency
branch
from
408e728 to
f771f8f
Compare
|
|
This solves #26 in making the
<Select>component consistent on web and native.For each
<Item>component, keep track of itsvaluetolabelmapping on the<Root>component by passing aMutableRefObjectthat stores the mapping from<Root>to<Item>via context.On render,
<Item>sets the label for its value.<Root>can then callonValueChangewithlabelForValueRef.current[val] ?? val, allowing feature parity with the native implementation.This approach works perfectly. Since
Itemmust be rendered before it can be interacted with and emit events, this always ensures thatlabelForValueRef[val]exists beforeonStrValueChangeis called. Furthermore, since thevalueanddefaultValueprop requires{ value: string, label: string }, the initial render also does not have any issues.