Skip to content

fix: Conditionally rendering fields in <StrictMode> #576

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

Conversation

fulopkovacs
Copy link
Contributor

Addresses #571.

TL;DR

It seems like the bug can be tracked to useField creating duplicate instances of the field (FieldApi) in strict mode and not cleaning them up properly. This PR at this point is just a proof of concept for a solution.

Details of the bug

Basically, the useField hook runs twice in strict mode, and creates a new instance of FieldApi every time it runs (source):

  const [fieldApi] = useState(() => {
    [...]
    
    const api = new FieldApi({
      ...opts,
      form: formApi as never,
      // TODO: Fix typings to include `index` and `parentFieldName`, if present
      name: name as typeof opts.name as never,
    })


    api.Field = Field as never


    return api
  })

I think that problem is that the cleanup function only runs once, because it's wrapped in a useIsomorphicEffectOnce hook source:

  useIsomorphicEffectOnce(() => {
    return () => {
      unmountFn.current?.()
    }
  })


  // We have to mount it right as soon as it renders, otherwise we get:
  // https://github.com/TanStack/form/issues/523
  if (!unmountFn.current) {
    unmountFn.current = fieldApi.mount()
  }

Honestly, there's a lot of things happening here, so I'm not a 100% percent sure I read the situation correctly. 😆

My solution and questions

I thought the best way to approach this issue is preventing a duplicate FieldInstance from being created, by checking if there's an instance already in the initialization function of the useState hook in useField. My question is: how dangerous is it? 🤣 In what cases would the field have multiple instances (in general, not just at this point)? Also, I'd love to understand a bit more about tanstack/store, especially if it's important here (a quick overview would be enough). I'm happy to get guidance on Discord, if a (or rather the) maintainer has time for it.

Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@crutchcorn
Copy link
Member

crutchcorn commented Feb 25, 2024

creates a new instance of FieldApi every time it runs

I'm not sure this is right. I thought, and I could be wrong, that a function passed to useState only ever executes once, even across duplicate effect runs.

Let me check and confirm this is the case. Either way, I wouldn't want to go down this path as I think there are other options available to us.

Edit: Yup, the useState function shouldn't run twice.

function App() {
  const [count, setCount] = useState(() => ({
    count: 1,
  }));
  const [_, rerender] = useReducer(() => ({}), {});

  return (
    <div>
      <button onClick={() => rerender()}>{count.count}</button>
      <button onClick={() => setCount((v) => ({ count: v.count++ }))}>
        {count.count}
      </button>
    </div>
  );
}

@fulopkovacs
Copy link
Contributor Author

Hmmm, I think the docs do say that the initializer function of a useState will be executed twice in Strict Mode:

In Strict Mode, React will call your initializer function twice in order to help you find accidental impurities. This is development-only behavior and does not affect production. If your initializer function is pure (as it should be), this should not affect the behavior. The result from one of the calls will be ignored. (docs)

And if we modify an external state in the initializer function, then we won't have a clean start for the second render. Check this (horrendous and client-only) modification of your example (see on StackBlitz):

import { useState, useReducer } from 'react';
import reactLogo from './assets/react.svg';
import viteLogo from '/vite.svg';
import './App.css';

function App() {
  const [count, setCount] = useState(() => {
    if (!window.executeCount) window.executeCount = 0;
    window.executeCount++;
    return {
      count: window.executeCount,
    };
  });
  const [_, rerender] = useReducer(() => ({}), {});

  return (
    <div>
      <button onClick={() => rerender()}>{count.count}</button>
      <button onClick={() => setCount((v) => ({ count: v.count++ }))}>
        {count.count}
      </button>
    </div>
  );
}

export default App;

For me, the counter shows 2 instead of 1 the first time I see it.

Is it really not possible that we have an impure initializer function for a useState hook in useField()? 🤔

Either way, I wouldn't want to go down this path as I think there are other options available to us.
🙌 Totally understandable, I was very new to the repo when I made this PR (still am), so I just tried the solution that was the most accessible for me, which is probably not the most optimal one 😅.

@crutchcorn
Copy link
Member

OMG you're right. I never knew about this 🫠🫠

Let me see if I can't figure out a different solution to this problem knowing this now

@crutchcorn
Copy link
Member

OK so things that don't seem to work include:

  • useRef outside of useState, early return if useRef is defined
  • Replacing useState with useMemo

I like your solution the best, but let's add it into:

#606 instead of this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants