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

Fix custom input's onChange handlers should have access to updated context value #8903

Closed
wants to merge 2 commits into from

Conversation

WiXSL
Copy link
Contributor

@WiXSL WiXSL commented May 11, 2023

I'm updating a project from ra-v3 to ra-v4 where the value of change inputs is taken from the Form state within the onChange / onBlur event handler (instead of its e.target.value argument), and it doesn't have the updated value.

I found out that :
In react-admin v3 inputs run custom event handlers after react-final-form ones.
In react-admin v4 inputs run custom event handlers before react-hook-form ones.

So, changing this order of execution to match ra-v3 allows useFormContext().getValues() to have the latest value inside the custom event handlers.

@slax57
Copy link
Contributor

slax57 commented May 12, 2023

Hi! Thanks for submitting this! 🙂

However, I'm not sure I understand, though. You can still access the newer value of the input via e.target.value, right? So why would you need to access it via useFormContext().getValues()? What is the benefit? 🤔

@WiXSL
Copy link
Contributor Author

WiXSL commented May 12, 2023

The benefit is to grab all the values of the form at once to give them to a process, including the one that just has been changed, without having to differentiate that one by grab it from another place.
I have a lot of code that takes all the values from the event handlers like that, and I have to go handler by handler taking the value that has changed from e.target.value plus the rest of the form values.

@slax57
Copy link
Contributor

slax57 commented May 12, 2023

I see. Thanks for the clarifications. 🙂
I was wondering, because this change is technically a BC.
However I can't think of any scenario where this would be problematic, so I would be OK to accept the change.
@djhi @fzaninotto what do you think? Can you think of a scenario where this might break things?

@slax57 slax57 requested review from fzaninotto and djhi May 12, 2023 14:42
Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is a welcome change, however I'd prefer that you PR against next. As it may break things in corner cases, we need to prevent that from happening in a bug fix release.

Also, the order change was introduced by @djhi in 9dfaf4e. I wonder it you have any recollection of why you didi so?

@WiXSL WiXSL changed the base branch from master to next May 15, 2023 13:05
@WiXSL WiXSL changed the base branch from next to master May 15, 2023 13:06
@WiXSL WiXSL closed this May 15, 2023
@WiXSL WiXSL deleted the fix-event-handler-exec-order branch May 15, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants