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

TextInput component not handling events during the initial life cycles #9670

Open
ylkhayat opened this issue Feb 16, 2024 · 4 comments
Open

Comments

@ylkhayat
Copy link

Some bit of context

We use react-admin version 4.16.9 in a pnpm monorepo workspace. And we have the automatic renovate pull requests to update our repository packages.
Our end to end tests were successful before updating to react-hook-form from 7.44.3 to 7.51.0 then it started to fail consistently. The failing test was related to react-admin's logging in page.

Test is as follows (in this order):

  • Cypress finds the username and password fields.
  • Cypress starts typing in the corresponding credentials.

Assume the credentials are:

  • username: a-user
  • password: what-a-password

What you were expecting:
It was expected to fill in the username and password fields normally. But we noticed that the test was failing because the typed in username was -user (notice the missing a) but the password field was normal what-a-password.

What happened instead:
It was expected of course that the username and password to have a-user and what-a-password respectively.

Steps to reproduce:

You can check the reproduction code here react-admin-delayed-sync.

If you change react-hook-form to 7.44.3 then you will see the sync works as expected

Version 7.44.3

react-hook-form-version-7.44.3.mov

Version 7.50.1

react-hook-form-version-7.50.1.mov

Thus, we can notice that the issue is with the TextInput component not from the registered input component with react-hook-form.

Related code:

You can check the sandbox that I have created for this react-admin-delayed-sync.

Environment

  • React-admin version: ^4.16.10
  • React version: 18.2.0
  • React Hook Form version: 7.50.1
  • Browser: Chrome
@ylkhayat ylkhayat changed the title TextInput component not handling events during the initial life cycle TextInput component not handling events during the initial life cycles Feb 16, 2024
@adguernier
Copy link
Contributor

Thanks for this report and the sandbox 🙏

However, tweaking the reproductible react-admin-sanbox by moving the document.querySelector('input[id="username-ra"]'); before the useEffect solves the issue you describe.

const raInput = document.querySelector('input[id="username-ra"]'); // it works
  useEffect(() => {
      ...

Knowing that the React way to get an element is to use useRef, I wonder if using document.querySelector instead of a ref could cause this issue (although for now , <TextInput> does not propagate to ref). 🤔

All things considered, for me this use case is unreliable to properly investigate and qualify this issue.
May you consider providing a project repository using the Cypress tests which lead to the issue you describe? Thanks

@ylkhayat
Copy link
Author

ylkhayat commented Feb 16, 2024

@adguernier you're absolutely correct, but I am not sure how did you make it work with a ref, that's definitely the first thing I have tried, so I had to hack it in somehow with the document.querySelector. I even tried with inputRef and it didn't work. Check the console.log() in the upcoming screenshot. But yeah if you only tried it with the document.querySelector then it's the same case.

image

And this is probably because of the useInput used in the component replaces the ref coming from the props and put in the ref coming in from react-hook-form hook. This could be solved by a merging of refs or something!

Note That!

And also regarding moving the document.querySelector line outside of the useEffect, it defies the purpose of this test, because the issue here is about handling events during the very initial life cycles. If you simply move it up one level then you will start typing exactly one event later, this case is when the return of the query selector becomes available but the useEffect diff didn't yet get the new value. But by putting in the querySelector in the useEffect it's a dynamic fetch (kinda).

Please have a look at the screen shots attached below.

Inside useEffect

Outside useEffect

image

Started typing at 2!

image

Started typing at 3!!

So I believe the issue is not about the retrieval of the component but rather the logic being handled inside the TextInput that receives the very first events from the browser but does not handle them properly!

Thanks for looking into this! And happy to help out!

@fzaninotto
Copy link
Member

Thanks for your report and the reproduction, which allows us to dig further.

After further inspection, it seems that the problem isn't linked to the onChange handler, which works fine in react-admin's TextInput, but to a reset of the form values on mount (or shortly after)

I tested it as follows:

  1. Open the CodeSandbox in full page (https://56pg7h-5173.csb.app/)
  2. Open the devtools, and find react-admin' useInput in the sources panel
  3. Add a breakpoint in useInput's onChange handler
  4. Refresh the screen
image

Initially, all 3 inputs have the first letter typed in ('a'), but after going one step further, the first 2 inputs have the correct value ('a-') but the third one only has '-'. This tends to show that react-admin resets the form a bit too late.

React-admin's Form sets the form default value based on the current RecordContext. The last time we've touched this mechanism is in #8911, nine months ago, and it was released in 4.11.0. There is no react-admin reason why it used to work on your side with a react-admin version > 4.11 and then stopped working.

So I tend to think it's a react-hook-form regression. To properly test it, you should set default values in your standalone RHF input.

Also, this makes me think that it doesn't make sense to use react-admin's Form in LoginForm instead of react-hook-form directly. We need none of the boilerplate of the Form component, which is designed with the use case of record forms.

@ylkhayat
Copy link
Author

@fzaninotto But this (LoginForm) is a black box to us, we don't have access to the LoginForm provided from react-admin and we'd like to keep using the built in form, without overriding the loginPage prop passed to the AdminUI wrapper, we just use it as it is.

Will this be handled from react-admin's side to simplify the LoginForm usage as you mentioned in an upcoming version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants