Minor form improvements

[cjreimer]

This PR addresses three minor items with the forms package:

1. If using on a field the validation={{valueAsNumber: true}} feature, and emptyAs is set to 'undefined', the react-hook-form would override the emptyAs. The solution is to remove the valueAsNumber internally in this case and rely on the setValueAs coersion we have in redwood.

2. If the name field is not set on a field component, the error would be deep from within react-hook-forms and unintelligible. A change is made to throw an intelligible error "name field must be provided". While typescript should normally help with this, not everyone uses typescript, and I have been burned by this one even using typescript.

3. Fixed a typescript warning relating to a ref in the test file.

[cjreimer]

@jtoar , what's the next step to get this reviewed? Thanks!

[Tobbe]

Thanks for your PR @cjreimer. I'll take a look as soon as I find some time (next few days hopefully)

[thedavidprice]

This is a great quality of life @cjreimer Thank you! And appreciate the nudge. You're in good hands with @Tobbe

[Tobbe]

1. If using on a field the validation={{valueAsNumber: true}} feature, and emptyAs is set to 'undefined', the react-hook-form would override the emptyAs. The solution is to remove the valueAsNumber internally in this case and rely on the setValueAs coersion we have in redwood.

✅ Confirmed and verified

2. If the name field is not set on a field component, the error would be deep from within react-hook-forms and unintelligible. A change is made to throw an intelligible error "name field must be provided". While typescript should normally help with this, not everyone uses typescript, and I have been burned by this one even using typescript.

✅ Confirmed and verified

3. Fixed a typescript warning relating to a ref in the test file.

Couldn't get TS to play nice in VS code for this one 🙁 I'll have to try again tomorrow because I need to go sleep now

EDIT: ✅ now confirmed and verified

[cjreimer]

Thanks, Tobbe, for reviewing!

[Tobbe]

@cjreimer Thanks for these changes. They work great when I test on a local project, but tests seems to be failing (also locally). Could you take a look at those failing tests please?

[cjreimer]

@Tobbe, thanks for reviewing.
Can you give your thoughts regarding removing the matches, that is:

import {
  toHaveFocus,
  toHaveClass,
  toBeInTheDocument,
} from '@testing-library/jest-dom/matchers'

and
expect.extend({ toHaveFocus, toHaveClass, toBeInTheDocument })

I believe this is the reason why the tests are failing now.

I believe we can get rid of these lines, as you propose, if we migrate to @testing-library/jest-dom to the v6 series. Note that we are currently at:
"@testing-library/jest-dom": "5.16.5",

I played with this, and I seem to remember I had this working with the forms package, but the CI checks complain if the redwood packages don't all match up (fair enough), so I wasn't sure if we wanted to do a major jest-dom upgrade in this small pr, and pulled that back, which meant returning those import lines and the expect.extend.

Thoughts?

[Tobbe]

Can you give your thoughts regarding removing the matches

@cjreimer It was giving TS errors, and removing it solved that. And it still built. So I thought things were still working. I read something somewhere about that stuff being automatically included. But as you said - might be we're on a too old version.

Feel free to revert my changes there if that gets things passing, and then we can loop back on fixing the red squiggles later

[cjreimer]

No problem. I just reverted all the matches, and the tests once again pass.
As you said, the upgrade to @testing-library/jest-dom can be a separate initiative.