-
Notifications
You must be signed in to change notification settings - Fork 3
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
add resolve-item callback to select-item-form-field #311
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,12 @@ export type SelectItemFormFieldProps< | |
SelectItemFormFieldInputProps<Item, SelectedItem>, | ||
'name' | 'label' | 'placeholder' | 'disabled' | ||
> & { | ||
/** | ||
* The function to resolve an item by its id. This will be called when the programmatically set value is not the same as the initial item. | ||
* When the function cannot resolve the item, it has to throw an error. | ||
* Make sure to pass a stable reference to prevent unnecessary calls. | ||
*/ | ||
resolveItem?: (id: ItemId) => Item | Promise<Item> | ||
renderInputItem?: (item: Item) => ReactNode | ||
extractIdFromItem: (item: Item) => ItemId | ||
/** | ||
|
@@ -69,6 +75,7 @@ export function SelectItemFormField< | |
mode, | ||
variant, | ||
initialItem, | ||
resolveItem, | ||
renderInputItem, | ||
label, | ||
placeholder, | ||
|
@@ -93,16 +100,66 @@ export function SelectItemFormField< | |
initialItem ?? null, | ||
) | ||
|
||
const fieldValueRef = useRef(field.value as ItemId) | ||
useEffect(() => { | ||
fieldValueRef.current = field.value as ItemId | ||
}, [field.value]) | ||
|
||
// This effect is used to set the selected item when the field value changes (i.e. when the value is set programmatically with React Hook Form). | ||
useEffect(() => { | ||
if (selectedItem && extractIdFromItem(selectedItem) === field.value) { | ||
return | ||
} | ||
|
||
// If the field value is null or undefined, we set the selected item to null. | ||
if (field.value === null || field.value === undefined) { | ||
setSelectedItem(null) | ||
} else if ( | ||
return | ||
} | ||
|
||
// If the field value is the same as the initial item's id, we set the selected item to the initial item. | ||
if ( | ||
initialItemRef.current && | ||
field.value === extractIdFromItem(initialItemRef.current) | ||
) { | ||
setSelectedItem(initialItemRef.current) | ||
return | ||
} | ||
}, [field.value, extractIdFromItem]) | ||
|
||
// At this point, we don't know the selected item. We have to resolve it with the resolveItem callback. | ||
|
||
if (!resolveItem) { | ||
console.warn( | ||
'SelectItemFormField: No resolveItem function provided. This is required to resolve the selected item.', | ||
) | ||
Comment on lines
+132
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we move such console logs behind a check that the NODE_ENV is in development mode? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to force the user of this component to specify this method, then I would make it mandatory and fix it in the projects. Currently we don't force it, but annoy you with a warning message. We should consider, that in 95% of our uses cases we don't need this function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it's not bad to always show it. Then you can see if there's a problem in production too.
That's why I would not force it. It makes it harder to use the component, and it can lead to a fake function being passed because there is no other way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I disagree here. In my opinion this is a log entry for a developer and therefore it should not be logged in a production environment.
I fully agree that it makes the component harder to use. On the other hand people have to actively ignore the warning message, which will be annoying and cause discussions. I would either allow to not define the function or force it. This is also linked to the From a technical point of view always forcing the function would be the best option, because only in that case the component is fully functioning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You only see the log entry when you open the dev tools, and that's when you really want to see logs. If there are no logs, you never have a chance to see what's wrong, even in production.
No, you only get this warning if
That's correct, yes. |
||
return | ||
} | ||
|
||
const fieldValueToBeResolved = field.value as ItemId | ||
|
||
void (async () => { | ||
try { | ||
const item = await resolveItem(fieldValueToBeResolved) | ||
|
||
// Make sure the field has not been changed in the meantime | ||
if (fieldValueToBeResolved === fieldValueRef.current) { | ||
// Make sure the resolved item is really the item that we want to set | ||
if (extractIdFromItem(item) === fieldValueToBeResolved) { | ||
setSelectedItem(item) | ||
} else { | ||
console.error( | ||
'SelectItemFormField: The resolved item could not be set as the selected item. It does not have the same id as the field value.', | ||
) | ||
} | ||
} | ||
} catch (error) { | ||
console.error( | ||
'SelectItemFormField: Error while resolving the selected item.', | ||
error, | ||
) | ||
} | ||
})() | ||
}, [field.value, selectedItem, extractIdFromItem, resolveItem]) | ||
|
||
return ( | ||
<> | ||
|
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.
Should we mark the
initialItem
property as deprecated? Do we want in the future only have the one way to resolve the information or is theinitialItem
concept still ok for us?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.
Although it's convenient for the developer to be able to set it at the beginning if we already have the entire item, it's probably better to have just one way to resolve it. Then we can get rid of this prop and we don't even need an initialValue prop because that's handled by the React hook form.
But I would suggest moving it to a new task/PR to deprecate it, because we need to try it to see if it works as expected.