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

[Bug]: Setting value on a captured reference to the set function on useControlField doesn't update the value #319

Open
1 of 4 tasks
jnicklas opened this issue Aug 30, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@jnicklas
Copy link
Contributor

Which packages are impacted?

  • remix-validated-form
  • @remix-validated-form/with-zod
  • @remix-validated-form/with-yup
  • zod-form-data

What version of these packages are you using?

latest

Please provide a link to a minimal reproduction of the issue.

https://codesandbox.io/p/sandbox/ecstatic-haze-y4wtdf?file=%2Fapp%2Froutes%2Findex.tsx%3A26%2C21

Steps to Reproduce the Bug or Issue

In the forked starter template, the first field uses useControlField and works as expected. The second field captures the setValue function with a useCallback and entering text into the field no longer works. The same problem happens when we use useMemo or any other function which captures a reference to the setter function. The same issue also seems to happen with useUpdateControlField as well.

This can cause really difficult to debug issues, since it is very easy with useEffect, useCallback and so on to accidentally use an "outdated" version of the setter function. It'd be really great if using an old version of it worked just fine.

In the starter template we also illustrated an ugly workaround using useRef. By using a ref and always setting it to the latest version of the setter function everything works as expected.

Expected behavior

I would want the setter function to work regardless of whether it has been captured in a previous render pass.

Screenshots or Videos

No response

Platform

  • OS: [e.g. macOS, Windows, Linux]
  • Browser: [e.g. Chrome, Safari, Firefox]
  • Version: [e.g. 91.1]

Additional context

No response

@jnicklas jnicklas added the bug Something isn't working label Aug 30, 2023
@moishinetzer
Copy link
Contributor

I'm also seeing the same issue when using clearError() in a useCallback

@airjp73
Copy link
Owner

airjp73 commented Sep 22, 2023

The fact that this hook mirrors useState's api probably contributes to the confusion here. With useState it's safe to leave the setter of of the deps array, but here it isn't. Currently, you need to do this:

const [value, setValue] = useControlField("myFieldName");
const callback = useCallback((value) => setValue(value), [setValue]);

It's definitely possible (and desirable) to change this. But it would require a bit of rearchitecting, so I can't promise a timeline.

A brief explanation of what's going on:
The form's zustand store is using a "slices"-esque pattern. The state for each form and the callbacks for updating the form's state get created for each form individually after the form is initially mounted. So on the first render we don't actually have a reference to the setValue function to provide via the hook. Therefore we use a no-op function on the first render and swap in the real one after.

This definitely has some downsides we can improve upon:

  • There's a minimum of two render passes when mounting a form
  • Each render pass has a different function reference for some callbacks (like you've brought up here).

To improve on this we have a couple options:

  • Move the state update callbacks up to the top-most level of the form store instead of creating them for each "slice". Then update all usages of these in the project.
  • Pull the callbacks out of the store using a ref to keep the function identity stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants