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

Update react-hook-form to 7.6.1 #2270

Closed
wants to merge 17 commits into from
Closed

Conversation

lunchbreakdev
Copy link
Contributor

Started updating react-hook-form to 7.0.7 to close #2253

Disclaimer: first contribution besides minor typo fixes 😂

I believe all necessary files are updated but I'm running into a configuration error with the tests. It appears to be occurring in the jest.config.js file, but I may have done something wrong.

Happy to make any other edits if I missed anything!

@github-actions
Copy link

github-actions bot commented Apr 13, 2021

📦 PR Packages

Click to Show Package Download Links

https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/create-redwood-app-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-api-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-api-server-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-auth-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-cli-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-core-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-dev-server-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-eslint-config-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-eslint-plugin-redwood-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-forms-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-internal-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-prerender-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-router-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-structure-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-testing-0.31.2-0170be3.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/2270/redwoodjs-web-0.31.2-0170be3.tgz

Install this PR by running yarn rw upgrade --pr 2270:0.31.2-0170be3

@thedavidprice thedavidprice requested a review from cannikin April 13, 2021 15:21
@thedavidprice
Copy link
Contributor

This is awesome @morganmspencer 🚀 Looping in @cannikin for review + direction.

@sriverag
Copy link

@morganmspencer - This looks awesome! I suggest including the yarn.lock file because the Cypress E2E tests check is installing the dependencies using yarn install --frozen-lockfile probably causing the tests to fail.

@thedavidprice
Copy link
Contributor

@morganmspencer could you update to the most recent React Hook Form version?
https://github.com/react-hook-form/react-hook-form/releases

pinging @cannikin for review

@lunchbreakdev lunchbreakdev changed the title Update react-hook-form to 7.0.7 Update react-hook-form to 7.3.6 May 1, 2021
@thedavidprice thedavidprice changed the title Update react-hook-form to 7.3.6 Update react-hook-form to 7.5.2 May 11, 2021
@thedavidprice
Copy link
Contributor

@morganmspencer I took a stab at getting this one into upcoming Redwood v0.32 — couldn't quite get it there:

  • bumped React-Hook-Form to 7.5.2
  • updated packages/forms/jest.config.js
  • combed through the RHF Changelog as they'd renamed a lot of the Types (without mentioning in Migration guide 🤦‍♂️)

CHANGELOG.md

Migration Guide

Status

The tests are now running although I'm seeing a strange error at the beginning regarding:

...
Error: Uncaught [TypeError: Cannot read property 'focus' of null]
...
  The above error occurred in the <TestComponentWithRef> component:
    at TestComponentWithRef (/Users/price/Repos/redwoodjs-redwood/packages/forms/src/__tests__/form.test.tsx:102:27)

Hopefully this is enough to get you going again?

@@ -268,7 +273,7 @@ const TextAreaField = forwardRef<
<textarea
{...tagProps}
id={props.id || props.name}
ref={(element) => {
{...(element: HTMLTextAreaElement | any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using any was the only type that didn't give an error. I'm no TS expert so assuming there's a better way to handle this.

Note: multiple occurrences below as well.

@lunchbreakdev
Copy link
Contributor Author

@thedavidprice it looks like the register function with the spread operator is returning the ref attribute (as well as onChange, onBlur and name). I think this may have been causing the error with ref, so I used the register call separately and defined the onChange and onBlur attributes explicitly.

I could be way off base with this way but 6 out of 12 tests now pass as opposed to the 3 from before. 🤷‍♂️

Would love any help with fixing the other tests.

@lunchbreakdev lunchbreakdev changed the title Update react-hook-form to 7.5.2 Update react-hook-form to 7.6.1 May 18, 2021
@cannikin
Copy link
Member

upgrading to v7 is critical path at this point

image

@thedavidprice
Copy link
Contributor

Feeling the pain 😭 --> #2402

@cannikin
Copy link
Member

Might be time to relax that "latest major version" requirement!

image

@jtoar
Copy link
Contributor

jtoar commented May 28, 2021

@cannikin while we're talking about it, what do you think of this one? #1434. If we think it's a good idea, we should probably do it all at the same time.


Update: just rediscovered the discussion here, seemed like we were all on board: #1431 (comment)

@jtoar
Copy link
Contributor

jtoar commented May 28, 2021

Note to self: these enums need to be changed (see the discussion here #2641 (comment)):

enum INPUT_TYPES {
BUTTON = 'button',
COLOR = 'color',
DATE = 'date',
DATETIME_LOCAL = 'datetime-local',
EMAIL = 'email',
FILE = 'file',
HIDDEN = 'hidden',
IMAGE = 'image',
MONTH = 'month',
NUMBER = 'number',
PASSWORD = 'password',
RADIO = 'radio',
RANGE = 'range',
RESET = 'reset',
SEARCH = 'search',
SUBMIT = 'submit',
TEL = 'tel',
TEXT = 'text',
TIME = 'time',
URL = 'url',
WEEK = 'week',
}

@lunchbreakdev
Copy link
Contributor Author

Note to self: these enums need to be changed (see the discussion here #2641 (comment)):

enum INPUT_TYPES {
BUTTON = 'button',
COLOR = 'color',
DATE = 'date',
DATETIME_LOCAL = 'datetime-local',
EMAIL = 'email',
FILE = 'file',
HIDDEN = 'hidden',
IMAGE = 'image',
MONTH = 'month',
NUMBER = 'number',
PASSWORD = 'password',
RADIO = 'radio',
RANGE = 'range',
RESET = 'reset',
SEARCH = 'search',
SUBMIT = 'submit',
TEL = 'tel',
TEXT = 'text',
TIME = 'time',
URL = 'url',
WEEK = 'week',
}

I just added a change for this

@jtoar
Copy link
Contributor

jtoar commented May 28, 2021

@morganmspencer awesome thanks!

@thedavidprice
Copy link
Contributor

@morganmspencer thanks for your willingness and patience with this PR! Assuming we can get someone to help guide you through the next steps, are you still available and interested in continuing with this?

@thedavidprice
Copy link
Contributor

thedavidprice commented Jun 9, 2021

General Note:
I suggest this PR also include the option 2 recommendation from this issue "Deprecate validation in <Form>" #1434

@lunchbreakdev
Copy link
Contributor Author

@thedavidprice I'm definitely still interested in working on this one with a bit of guidance

@thedavidprice
Copy link
Contributor

@morganmspencer thanks! I've asked @dthyresson to take a look and check back here. Keep a 👀!

@thedavidprice
Copy link
Contributor

Hi @morganmspencer We are going to close this PR and pick up the work in #3043

Appreciate all your help (and patience) and welcome you to join in on the new PR if you'd like. No pressure! Just know your help is always welcome.

@redwoodjs-bot redwoodjs-bot bot removed this from the next-release-priority milestone Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change topic/forms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forms: Upgrade React Hook Form to v7
5 participants