Skip to content
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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
} from '../../../../.storybook/components'
import { ErrorBody } from '../../util'
import { Form } from '../Form'
import { Button } from '../../button'
import {
SearchQueryParameters,
PaginatedResponse,
Expand All @@ -40,9 +41,10 @@ const meta = {
dialogTitle: 'Users',
dialogLabel: 'Users',
noSearchResults: 'No users available.',
extractIdFromItem: (item) => (item as User).id,
renderListItem: (item) => (item as User).name,
renderErrorMessage: (error) => (error as ErrorBody).message,
resolveItem,
extractIdFromItem: (item) => item.id,
renderListItem: (item) => item.name,
renderErrorMessage: (error) => error.message,
paginationConfig: { indexType: IndexType.ZERO_BASED },
dialogProps: { overlayClassName: 'z-10' },
},
Expand All @@ -56,16 +58,26 @@ const meta = {
})
type Person = z.infer<typeof personSchema>
const defaultPerson: DefaultValues<Person> = {
userId: (context.args.initialItem as User | undefined)?.id,
userId: context.args.initialItem?.id,
}
const form = useForm({
resolver: zodResolver(personSchema),
defaultValues: defaultPerson,
})
return (
<Form form={form} onSubmit={action('onSubmit')}>
<Story />
</Form>
<>
<Form form={form} onSubmit={action('onSubmit')}>
<Story />
</Form>
<Button
className="mt-4"
onClick={() => {
form.setValue('userId', 999)
}}
>
Set to `User 999`
</Button>
</>
)
},
],
Expand All @@ -91,10 +103,10 @@ const meta = {
),
},
},
} satisfies Meta<typeof SelectItemFormField>
} satisfies Meta<typeof SelectItemFormField<User, User, number, ErrorBody>>

export default meta
type Story = StoryObj<typeof SelectItemFormField>
type Story = StoryObj<typeof meta>

const useGetDataSuccess = ({
search,
Expand Down Expand Up @@ -170,6 +182,17 @@ const useGetDataEmpty = ({
}
}

function resolveItem(id: number): Promise<User> {
return new Promise((resolve) =>
setTimeout(() => {
resolve({
id,
name: `User ${id.toString()}`,
})
}, 200),
)
}

export const Default: Story = {
args: { useGetData: useGetDataSuccess },
}
Expand Down
Copy link
Member

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 the initialItem concept still ok for us?

Copy link
Contributor Author

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.

Original file line number Diff line number Diff line change
Expand Up @@ -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
/**
Expand Down Expand Up @@ -69,6 +75,7 @@ export function SelectItemFormField<
mode,
variant,
initialItem,
resolveItem,
renderInputItem,
label,
placeholder,
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

I think it's not bad to always show it. Then you can see if there's a problem in production too.

We should consider, that in 95% of our uses cases we don't need this function.

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.

Copy link
Member

@mmalfertheiner mmalfertheiner Apr 30, 2024

Choose a reason for hiding this comment

The 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.

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.

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.

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 initialItem topic below, which would force you to set it all cases.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

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.

On the other hand people have to actively ignore the warning message, which will be annoying cause discussions

No, you only get this warning if resolveItem is required because you are actively setting the value of the field. If you don't set it (95% of the time), you won't see this warning.

This is also linked to the initialItem topic below, which would force you to set it all cases.

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 (
<>
Expand Down