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 #7018

Merged
merged 20 commits into from
Jul 18, 2023
Merged

Update react hook form #7018

merged 20 commits into from
Jul 18, 2023

Conversation

szymonlesisz
Copy link
Contributor

@szymonlesisz szymonlesisz commented Nov 24, 2022

Description

Someone finally has to do it :)

Kicking of react-hook-form update and looking for help

Breaking changes:

https://github.com/react-hook-form/react-hook-form/blob/master/CHANGELOG.md

Related to:
resolves #6596

i guess first we should update react-hook-form, test if working correctly and then update react-testing-library

@szymonlesisz szymonlesisz changed the title Chore/update react hook form WIP: update react hook form Nov 24, 2022
@@ -13,7 +13,7 @@
"dependencies": {
"@suite-native/atoms": "*",
"react": "18.2.0",
"react-hook-form": "6.15.7",
"react-hook-form": "7.39.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

I will update suite-native part once you are done with desktop but should be quick, we have react-hook-form abstracted so we do not access API directly

@socket-security
Copy link

socket-security bot commented May 29, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@komret komret force-pushed the chore/update-react-hook-form branch from 1d40d53 to 5514467 Compare May 29, 2023 15:46
@komret komret force-pushed the chore/update-react-hook-form branch from 5514467 to f8c7d03 Compare June 14, 2023 11:54
@socket-security
Copy link

socket-security bot commented Jun 14, 2023

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@hookform/resolvers 1.3.8...3.1.0 None +3/-3 3.04 MB bluebill1049
react-hook-form 6.15.7...7.44.3 network +0/-0 850 kB bluebill1049

@komret komret force-pushed the chore/update-react-hook-form branch 3 times, most recently from 8484f4f to 85958ae Compare June 20, 2023 09:23
@komret komret force-pushed the chore/update-react-hook-form branch 2 times, most recently from 6a08a09 to 0da09e4 Compare June 22, 2023 12:22
@komret komret added send Send page dependencies Pull requests that update a dependency file +Invity Related to Invity project labels Jun 22, 2023
@komret komret changed the title WIP: update react hook form Update react hook form Jun 22, 2023
@komret komret force-pushed the chore/update-react-hook-form branch 3 times, most recently from 9440271 to 0334611 Compare June 22, 2023 14:19
@komret
Copy link
Contributor

komret commented Jun 22, 2023

Mostly done. I am investigating why some tests are failing, any insight appreciated - @dahaca, @szymonlesisz...

@trezor/invity-suite This upgrade involves a lot of changes to the coinmarket section of Suite, can you please also take a look at the PR?

@Nodonisko Minor changes to the suite-native package were also necessary due to changes in RHF's TS definitions.

Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

According to migration docs and what I know about forms, it looks good imho. There are some problems with invity forms ⬇️

No idea regarding the useRbfForm test

Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

The e2e coinmarket buy form is failing correctly.

I am not able to choose address

Buy BTC for 5k kč, choose Banxa, bank transfer. Try to select btc address.

Screen.Recording.2023-06-23.at.11.32.25.mov

Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

Sell form with small amount worked weird before. Now it misses error

Before:

Screen.Recording.2023-06-23.at.11.36.18.mov

I have to interact with form again to see the error

Now:

Screen.Recording.2023-06-23.at.11.37.18.mov

Interaction does not help

In Exchange tab it worked before correctly (immediatelly), now it does not show the error

Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

If I fill 0 i get an error in console. In production, I get error in form
Screenshot 2023-06-23 at 12 34 44

@szymonlesisz
Copy link
Contributor Author

szymonlesisz commented Jun 26, 2023

i've checked failing tests for useSendForm and useRbfForm hooks. following changes has to be made to make it work:

  1. useAsyncDebounce timeout is never cleared once triggered. therefore compose process is called even after the component is unmounted (test is finished)
    Clearing the timeout on unmount should help:
useEffect(
        () => () => {
            if (timeout.current) clearTimeout(timeout.current);
        },
        [],
 );
  1. CustomFee component returns Translation in validate function. i should work when it will return string.

  2. useCompose hook triggers compose process multiple times by changing the value of feeLimit to undefined
    Set fallback it to empty string in case when feeLimit in not present in composed tx (all btc like coins, only eth is working with feeLimit field)

setValue('feeLimit', feeLimit || '');

@komret komret force-pushed the chore/update-react-hook-form branch 2 times, most recently from 84f3e77 to e2e5e5d Compare July 3, 2023 11:00
@komret
Copy link
Contributor

komret commented Jul 15, 2023

As for the failing e2e test, it shouldn't matter. The field changed its name from outputs[0].address to outputs.0.address. The migration test checks the name on https://suite.corp.sldev.cz/suite-web/release/22.7/web/accounts/send where it hasn't been updated yet.

@ondracja FYI

@komret
Copy link
Contributor

komret commented Jul 15, 2023

@LukasRada Can I merge the PR?

@LukasRada
Copy link
Contributor

@komret Yes.

@matejkriz
Copy link
Member

/rebase

@github-actions
Copy link

@trezor-ci trezor-ci force-pushed the chore/update-react-hook-form branch from 6757e84 to 166b2cc Compare July 18, 2023 07:40
@matejkriz matejkriz merged commit a110631 into develop Jul 18, 2023
@matejkriz matejkriz deleted the chore/update-react-hook-form branch July 18, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file +Invity Related to Invity project send Send page
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants