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

feat: removal of react hook form #166

Merged
merged 21 commits into from
Dec 14, 2023
Merged

Conversation

DNR500
Copy link
Contributor

@DNR500 DNR500 commented Dec 11, 2023

Jira: LF-5981

This PR looks to remove the react-hook-form library from the Widget

It puts a zustand store in its place to handle the passing of form values across the app.

Main hooks that interact with the form store

  • useFieldValue - now provides access to the form values
  • useFieldActions - provides functions to update the form
  • useFieldController - provides an interface with onChange, onBlur and value to hook up to inputs

other hooks

  • useTouchedFields - provides a map which fields are touched
  • *useValidation - provides access to isValid and errors
  • *useValidationActions - functions to set up and trigger the validatoiin

*The validation here is a placeholder and will likely change when looking at the new changes for the Wallet Switcher (should be the next piece of work)

@DNR500 DNR500 added the WIP Work in progress label Dec 11, 2023
@chybisov chybisov changed the title Feat: removal of react hook form feat: removal of react hook form Dec 11, 2023
@DNR500 DNR500 force-pushed the spike-removal-of-react-hook-form branch from 19e96e5 to 00ae41b Compare December 12, 2023 18:22
@DNR500
Copy link
Contributor Author

DNR500 commented Dec 12, 2023

We might want to revise what hooks are made available for the external API?

see commit 44b71ec

@DNR500 DNR500 force-pushed the spike-removal-of-react-hook-form branch from 0dad7bf to b6e6450 Compare December 12, 2023 18:48
@DNR500 DNR500 removed the WIP Work in progress label Dec 12, 2023
@DNR500 DNR500 marked this pull request as ready for review December 12, 2023 18:50
Copy link
Member

@chybisov chybisov left a comment

Choose a reason for hiding this comment

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

I couldn't find URLSearchParamsBuilder event though we export it from form barrel file.

Comment on lines 5 to 10
// TODO: needs to be removed
declare global {
interface Window {
useFormStore?: boolean;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes. I'll get rid of that

Comment on lines 172 to 173
// The following a is a "placeholder" "for validation for toAddress
// is will be re-addressed in the next piece of work for Wallet Switching
Copy link
Member

Choose a reason for hiding this comment

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

Can we please improve this comment? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we can probably live without it - I'll delete it

{ ...valuesToFormValues(formDefaultValues) },
);

// TODO: isValidating & isValid
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is done?

Comment on lines +53 to +72
useEffect(() => {
addFieldValidation('toAddress', async (value: string) => {
try {
if (!value && requiredToAddress) {
return t('error.title.walletEnsAddressInvalid');
}
},
onBlur: () => trigger(FormKey.ToAddress),
},
});

if (!value || isAddress(value) || isSVMAddress(value)) {
return true;
}
const address = await getEnsAddress(config, {
chainId: getFieldValues('toChain')[0],
name: normalize(value),
});
return Boolean(address);
} catch (error) {
return t('error.title.walletEnsAddressInvalid') as string;
}
});
}, [addFieldValidation, requiredToAddress, getFieldValues, t, config]);
Copy link
Member

Choose a reason for hiding this comment

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

Should we add this field onBlur instead of creating validation on every field update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this and decide for now its actually ok for it to stay here as it only really will update on requiredToAddress and config changing - though we might look to optimise the pattern for validation in future.

When testing this is wasn't actually working. This was main due to a problem in the useValidationActions hook and a missing call to clearErrors when valid.

Now fixed and works as it did previously - the additional clearErrors logic has been put in the formStore logic and the useValidationActions has been rewritten like the useFieldActions hook so that it avoid any potential problems related to connascence of position/meaning. See commit 9ebef7f

@@ -1,10 +1,8 @@
import { useEffect, useRef, useState } from 'react';
import { useWatch } from 'react-hook-form';
import { useFieldValues } from '../stores';

export const useDebouncedWatch = (name: any, delay: number) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove any and type name properly here now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually yes and I think the use of the useDebouncedWatch probably doesn't need to be used with FormKey anymore

Comment on lines 49 to 50
// Makes widget config options reactive to changes
// Acts similar to values property from useForm, but includes additional logic for chains
Copy link
Member

Choose a reason for hiding this comment

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

Should be updated I guess, we no longer have useForm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update

Comment on lines 1 to 2
import type { UseBoundStoreWithEqualityFn } from 'zustand/esm/traditional';
import type { StoreApi } from 'zustand/esm';
Copy link
Member

Choose a reason for hiding this comment

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

I think ESM is used by default, so we can safely import from zustand and zustand/traditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check those and change them

@chybisov
Copy link
Member

We might want to revise what hooks are made available for the external API?

see commit 44b71ec

Maybe later, those are undocumented exports for our experimental NFT flow.

@DNR500
Copy link
Contributor Author

DNR500 commented Dec 13, 2023

I couldn't find URLSearchParamsBuilder event though we export it from form barrel file.

I've fixed the gitignore file now, this was filtering out the URLSearchParamsBuilder.tsx file and listing it as ignored due to **build**. The change to the gitignore file also changes how dist folders are ignored. I've checked that this works locally.

I think this should be ready for review again

@DNR500 DNR500 added the testing label Dec 14, 2023
Copy link

Hey! This is your new endopint: https://6ab298ef.widget-spikeremov.pages.dev

@chybisov chybisov merged commit 3356386 into main Dec 14, 2023
2 checks passed
@chybisov chybisov deleted the spike-removal-of-react-hook-form branch December 14, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants