-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
override useShowController onFailure behavior and return error object #6625
Conversation
… also added "error" to data returned object
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.
Hi and thanks for taking the time to open a PR!
Although adding the onFailure
is ok, it's a new feature so your PR should target the next
branch. However, please revert the addition of the error
returned by useGetOne
and useShowController
. Indeed, overriding the onFailure
should be enough as it will called with the error as its first argument even though we don't use it in our default handler.
Finally, can you please revert unrelated changes in the documentation?
Thanks!
@djhi , The reason i added "error" property is to be able to support the following usecase: const showController = useShowController<T>({
resource, basePath, id,
onFailure: () => {}, // prevent redirect to list and user notification on errors
})
const { loaded, record, error } = showController
if (error) return <Error error={error} />
else if (loaded) return <Render record={record} />
else return <Loading /> This way, I do not have to create custom |
@djhi , can we commit with |
@panfiva We discussed it and we agree it is a good idea so thank you for bringing that up! However, there are two other places where this would make sense as well: |
I can add support there as well. Would you like me to update the current pull request, or create a separate pull request? |
Thanks! You can do that in the same PR if you want. |
Thanks for starting the work on this! I finished it in another PR as we want to release |
See #5197