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

Rules of Controller does not react to changes #1749

Closed
joaoeudes7 opened this issue Jun 1, 2020 · 36 comments
Closed

Rules of Controller does not react to changes #1749

joaoeudes7 opened this issue Jun 1, 2020 · 36 comments
Labels
question Further information is requested

Comments

@joaoeudes7
Copy link

joaoeudes7 commented Jun 1, 2020

First, the lib is very cool!! Thanks!

My problem is in set the rule in Controller:

{{ required: needRegister, min: 3 }}

The property rules of Controller does not not react to needRegister

Codesandbox link (Required)

Expected behavior
React to changes in rules

Desktop (please complete the following information):

  • OS: Arch Linux Amd64
  • Browser Firefox
  • Version 77
@bluebill1049 bluebill1049 added the question Further information is requested label Jun 1, 2020
@bluebill1049
Copy link
Member

bluebill1049 commented Jun 1, 2020

we cache the rules object inside the controller, hence the validation rules is not changing. (so the user doesn't have to memo the rules object themself)

Solutions

  • use validate function combined getValues() (assume the toggle input is register with hook form as well)
  • unregister that input, so input gets re-registered with updated rules.

@acfasj
Copy link

acfasj commented Jun 2, 2020

we cache the rules object inside the controller, hence the validation rules is not changing. (so the user doesn't have to memo the rules object themself)

Solutions

  • use validate function combined getValues() (assume the toggle input is register with hook form as well)
  • unregister that input, so input gets re-registered with updated rules.

seems like my validate function was cached, too. and re-registered looks bad

@kotarella1110
Copy link
Member

@bluebill1049 Controlled Input and Uncntrolled Input are inconsistent. Shouldn't this be fixed?

https://codesandbox.io/s/react-hook-form-controller-template-oolpt

@bluebill1049
Copy link
Member

we cache the rules object inside the controller, hence the validation rules is not changing. (so the user doesn't have to memo the rules object themself)
Solutions

  • use validate function combined getValues() (assume the toggle input is register with hook form as well)
  • unregister that input, so input gets re-registered with updated rules.

seems like my validate function was cached, too. and re-registered looks bad

You mean unregister at useEffect?

@bluebill1049
Copy link
Member

bluebill1049 commented Jun 2, 2020

@bluebill1049 Controlled Input and Uncntrolled Input are inconsistent. Shouldn't this be fixed?

https://codesandbox.io/s/react-hook-form-controller-template-oolpt

happy to see a simple solution for this. 🙏 basically we need to re-register the input if rules changing. however, we didn't want users to cache/memo the rules. Simplest is just to unregister controller and let it re-register again. otherwise, we will need some compare to run each re-render's rules or re-register controller each render.

@kotarella1110
Copy link
Member

@bluebill1049 I think should be fix the line below. we should custom compare deps (rulesRef).

https://github.com/react-hook-form/react-hook-form/blob/v5.7.2/src/controller.tsx#L110

@bluebill1049
Copy link
Member

@bluebill1049 I think should be fix the line below. we should custom compare deps (rulesRef).

https://github.com/react-hook-form/react-hook-form/blob/v5.7.2/src/controller.tsx#L110

yea, hopefully not going to introduce too much code for this change. I did have a note on the documentation on this too that we cache the rules.

We do have compareObject method, but then you will need deep compare with validating function which is not going to be pretty and light weight compare.

@kotarella1110
Copy link
Member

#1749 (comment)

You're right. This may not be light weight compare.

@bluebill1049
Copy link
Member

bluebill1049 commented Jun 2, 2020

#1749 (comment)

You're right. This may not be light weight compare.

I think we should take this as a tradeoff and documented the solution and attach that under the rules section.

  • external state: leverageunregister.
  • local input state: use validate function with getValues() to read other inputs' value .

🤔 an alternative solution will be on users to memo all the rules, which is not great DX.

const rules1 = useMemo(() => rules, [....]])
const rules2 = useMemo(() => rules, [....]])

<Controller rules={rule1} />
<Controller rules={rule2} />

@bluebill1049 bluebill1049 added improve documentation documentation update required and removed question Further information is requested labels Jun 2, 2020
@bluebill1049 bluebill1049 self-assigned this Jun 2, 2020
@acfasj
Copy link

acfasj commented Jun 2, 2020

we cache the rules object inside the controller, hence the validation rules is not changing. (so the user doesn't have to memo the rules object themself)
Solutions

  • use validate function combined getValues() (assume the toggle input is register with hook form as well)
  • unregister that input, so input gets re-registered with updated rules.

seems like my validate function was cached, too. and re-registered looks bad

You mean unregister at useEffect?

yes, here is the code in my project:

useEffect(() => {
    promotion && unregister('number')
  }, [promotion, unregister])

I have to do some explanations why I have add a useEffect here, bacause it looks werid in the “business logic” code. (I event dont really understand why unregister it can make it work, because unregister trigger something in RHF so it re-registered again ?)

and another common case is, form validation, offen need some “context” information, that I have to fetch something customer specific details to determin final rules, so make rules react to the props or something similar feel more proper to me

@bluebill1049
Copy link
Member

Thanks for your feedback. have a read on the note above first to understand the rationale behind: #1749 (comment)

I have to do some explanations why I have add a useEffect here, bacause it looks werid in the > “business logic” code. (I event dont really understand why unregister it can make it work, because unregister trigger something in RHF so it re-registered again ?)

Because rules are cached, so once it's unregistred, it will get registered at the render with updated rules.

and another common case is, form validation, offen need some “context” information, that I have to fetch something customer specific details to determin final rules, so make rules react to the props or something similar feel more proper to me

if it's coming down as props, then same use case with unregister, if it's trigger by user input, you can use validate and getValues.

Screen Shot 2020-06-02 at 3 00 13 pm

@bluebill1049
Copy link
Member

bluebill1049 commented Jun 2, 2020

Summarise

We have the following options:

  1. register on each render without cache rules
  • con: perf and why you do not change rules, it's an overkill
  • prop: work when you change rules
  1. let users cache their own rules
const rules1 = useMemo(() => rules, [....]])
const rules2 = useMemo(() => rules, [....]])
<Controller rules={rule1} />
<Controller rules={rule2} />
  • con: not great DX
  • prop: works and good perf
  1. cache the rules
  • con: update rules are not great DX
  • prop: perf and have work-around

3 ✅(what we choose at the moment, cheap and have work-around)

@bluebill1049
Copy link
Member

Sorry deleted previous message...

I think i found an easier solution: you can update the rules by invoking register again.

https://codesandbox.io/s/controller-rules-8pd7z?file=/src/App.tsx

let me know if this helps.

@bluebill1049
Copy link
Member

I have updated the doc and will push it alone with V6: react-hook-form/documentation@39663da

@bluebill1049
Copy link
Member

closing this issue, documentation will be released as part of V6 update.

@joaoeudes7
Copy link
Author

Thanks 😄

@pedroguia
Copy link

Sorry deleted previous message...

I think i found an easier solution: you can update the rules by invoking register again.

https://codesandbox.io/s/controller-rules-8pd7z?file=/src/App.tsx

let me know if this helps.

I have a similar problem to op, "required" rule must be dynamic and on top of that i need to use setValue() on the field that was re-registered.

Your last solution solves the required problemas but stops me from changing the value, i guess it changes but it is not visible on the textfield:
https://codesandbox.io/s/controller-rules-npe18?file=/src/App.tsx

Any ideia?

@bluebill1049
Copy link
Member

Sorry deleted previous message...
I think i found an easier solution: you can update the rules by invoking register again.
https://codesandbox.io/s/controller-rules-8pd7z?file=/src/App.tsx
let me know if this helps.

I have a similar problem to op, "required" rule must be dynamic and on top of that i need to use setValue() on the field that was re-registered.

Your last solution solves the required problemas but stops me from changing the value, i guess it changes but it is not visible on the textfield:
https://codesandbox.io/s/controller-rules-npe18?file=/src/App.tsx

Any ideia?

why so complicated?

            register("localState", {
              required: e.target.checked
            });
            setValue("localState", e.target.checked.toString()); // added this line
            setLocalState(e.target.checked);

u r doing both controlled and uncontrolled, choose one.

@pedroguia
Copy link

I changed the previous code as much as i could to simulate my real problem.
Can you take a look? https://codesandbox.io/s/controller-rules-npe18?file=/src/App.tsx

As you can see, required erros appear even though the fields are not required in that moment.
Thats why i thought on re-registering both hour fields whenever the switched are toggled.
Thanks.

@bluebill1049
Copy link
Member

I changed the previous code as much as i could to simulate my real problem.
Can you take a look? https://codesandbox.io/s/controller-rules-npe18?file=/src/App.tsx

As you can see, required erros appear even though the fields are not required in that moment.
Thats why i thought on re-registering both hour fields whenever the switched are toggled.
Thanks.

Taking a quick glance looks like the wrong usage, why are you set values in there

const handleChangeAllDay = ([, value]) => {
    const startHour = value ? "00:00" : "";
    const endHour = value ? "23:59" : "";
    setValue([{ startHour }, { endHour }]);
    if (value) setValue([{ periodic: false }]);

    return value;
  };

personally I would build an entire component that includes all those fields and wrap with the controller. when you using controller and then manually use setValue it raise concern to me right away.

@bluebill1049
Copy link
Member

is this codesandbox works for you? https://codesandbox.io/s/controller-rules-8pd7z?file=/src/App.tsx @pedroguia if so, maybe try to follow the same pattern.

@pedroguia
Copy link

Cant build a component that includes all of these fields because they are already in different bigger components inside my project.

I've already tried that approach and coudnt make it work, bu ill try again.
Using controller and setValue together as work as expected for the entire form, except for this little problem.

@bluebill1049
Copy link
Member

Cant build a component that includes all of these fields because they are already in different bigger components inside my project.

I've already tried that approach and coudnt make it work, bu ill try again.
Using controller and setValue together as work as expected for the entire form, except for this little problem.

ok, I will take a look closer during lunch time.

@bluebill1049 bluebill1049 reopened this Jun 3, 2020
@bluebill1049 bluebill1049 removed the improve documentation documentation update required label Jun 3, 2020
@bluebill1049 bluebill1049 added the question Further information is requested label Jun 3, 2020
@joaoeudes7
Copy link
Author

joaoeudes7 commented Jun 4, 2020

Ops! Validate do not work with values of useState, and defaultValues() dot not set values of getValues() :/
Does the development version already support this?

@bluebill1049
Copy link
Member

Ops! Validate do not work with values of useState, and defaultValues() dot not set values of getValues() :/
Does the development version already support this?

do you have a codesandbox for this?

@acfasj
Copy link

acfasj commented Jun 8, 2020

if anyone's situation is simply depend on a prop, and it change only once, maybe use key prop to force Controller recreate the component helps

<Controller
  name='number'
  control={control}
  as={InputNumber}
  defaultValue={1}
  // @see https://github.com/react-hook-form/react-hook-form/issues/1749
  key={promotion ? 'YES' : 'NO'}
  rules={{
    required: '必填',
    min: {
      value: 1,
      message: '不能小于1',
    },
    validate: (value: number) => {
      if (!hasAttended && promotion) {
        const valid = value <= promotion.joinMax

@bluebill1049
Copy link
Member

@acfasj we are fixing this next release.

@safonovklim
Copy link

safonovklim commented Jun 11, 2020

I also have this problem due to a recent upgrade from 5.3.x to 5.7.x

Is it possible to add flag rulesCache={false} to <Controller />?

Rules work fine without <Controller/>: https://codesandbox.io/s/react-hook-form-custom-validation-simple-92crr

Rules work differently with <Controller/>: https://codesandbox.io/s/react-hook-form-custom-validation-with-controller-5rdli

@bluebill1049
Copy link
Member

I also have this problem due to a recent upgrade from 5.3.x to 5.7.x

Is it possible to add flag rulesCache={false} to <Controller />?

Rules work fine without <Controller/>: https://codesandbox.io/s/react-hook-form-custom-validation-simple-92crr

Rules work differently with <Controller/>: https://codesandbox.io/s/react-hook-form-custom-validation-with-controller-5rdli

we are fixing the in v6, no extra props required.

@hatchli
Copy link

hatchli commented Jun 13, 2020

Ops! Validate do not work with values of useState, and defaultValues() dot not set values of getValues() :/
Does the development version already support this?

This is exactly my issue as well. Values of useState are not respected by Validate. Will this be what is addressed in v6 @bluebill1049?

@bluebill1049
Copy link
Member

Ops! Validate do not work with values of useState, and defaultValues() dot not set values of getValues() :/
Does the development version already support this?

This is exactly my issue as well. Values of useState are not respected by Validate. Will this be what is addressed in v6 @bluebill1049?

Yes, correct. right now you have to follow what i did above for a work around.

@hatchli
Copy link

hatchli commented Jun 14, 2020

Ops! Validate do not work with values of useState, and defaultValues() dot not set values of getValues() :/
Does the development version already support this?

This is exactly my issue as well. Values of useState are not respected by Validate. Will this be what is addressed in v6 @bluebill1049?

Yes, correct. right now you have to follow what i did above for a work around.

This? Okay, thanks @bluebill1049 ! I have the luxury of waiting for the release, so I'll do that for now!

@bluebill1049
Copy link
Member

@bluebill1049
Copy link
Member

we will have another RC version before the final release of v6. (make sure the quality is good)

@YuZaiShui
Copy link

Thanks for your feedback. have a read on the note above first to understand the rationale behind: #1749 (comment)

I have to do some explanations why I have add a useEffect here, bacause it looks werid in the > “business logic” code. (I event dont really understand why unregister it can make it work, because unregister trigger something in RHF so it re-registered again ?)

Because rules are cached, so once it's unregistred, it will get registered at the render with updated rules.

and another common case is, form validation, offen need some “context” information, that I have to fetch something customer specific details to determin final rules, so make rules react to the props or something similar feel more proper to me

if it's coming down as props, then same use case with unregister, if it's trigger by user input, you can use validate and getValues.

Screen Shot 2020-06-02 at 3 00 13 pm

Thanks for your feedback. have a read on the note above first to understand the rationale behind: #1749 (comment)

I have to do some explanations why I have add a useEffect here, bacause it looks werid in the > “business logic” code. (I event dont really understand why unregister it can make it work, because unregister trigger something in RHF so it re-registered again ?)

Because rules are cached, so once it's unregistred, it will get registered at the render with updated rules.

and another common case is, form validation, offen need some “context” information, that I have to fetch something customer specific details to determin final rules, so make rules react to the props or something similar feel more proper to me

if it's coming down as props, then same use case with unregister, if it's trigger by user input, you can use validate and getValues.

Screen Shot 2020-06-02 at 3 00 13 pm
unregister input at useEffect and let Controller re-register itself with updated rules.

I have a question, does that mean I don't have to re-register manually, the Controller will re-register automatically. And in my opinion, re-register is a hack way of writing that does break the existing internal state of a control, such as dirty, touch, etc

@bluebill1049
Copy link
Member

Thanks for your feedback. have a read on the note above first to understand the rationale behind: #1749 (comment)

I have to do some explanations why I have add a useEffect here, bacause it looks werid in the > “business logic” code. (I event dont really understand why unregister it can make it work, because unregister trigger something in RHF so it re-registered again ?)

Because rules are cached, so once it's unregistred, it will get registered at the render with updated rules.

and another common case is, form validation, offen need some “context” information, that I have to fetch something customer specific details to determin final rules, so make rules react to the props or something similar feel more proper to me

if it's coming down as props, then same use case with unregister, if it's trigger by user input, you can use validate and getValues.
Screen Shot 2020-06-02 at 3 00 13 pm

Thanks for your feedback. have a read on the note above first to understand the rationale behind: #1749 (comment)

I have to do some explanations why I have add a useEffect here, bacause it looks werid in the > “business logic” code. (I event dont really understand why unregister it can make it work, because unregister trigger something in RHF so it re-registered again ?)

Because rules are cached, so once it's unregistred, it will get registered at the render with updated rules.

and another common case is, form validation, offen need some “context” information, that I have to fetch something customer specific details to determin final rules, so make rules react to the props or something similar feel more proper to me

if it's coming down as props, then same use case with unregister, if it's trigger by user input, you can use validate and getValues.
Screen Shot 2020-06-02 at 3 00 13 pm

unregister input at useEffect and let Controller re-register itself with updated rules.
I have a question, does that mean I don't have to re-register manually, the Controller will re-register automatically. And in my opinion, re-register is a hack way of writing that does break the existing internal state of a control, such as dirty, touch, etc

V6 will update rules each render.

@bluebill1049 bluebill1049 removed their assignment Aug 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

8 participants