facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
225.47k stars 45.97k forks source link

Keys for hooks #14998

Closed jamesknelson closed 5 years ago

jamesknelson commented 5 years ago

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

Hooks must always be called in the same order.

What is the desired behavior?

It would be useful to be able to use hooks in loops and conditions, as this would allow hooks to be lifted out of child components that are rendered by loops and conditions.

For example, consider this hook that stores the state for a contact form:

function useContactModel({ defaultValue }) {
  let [name, setName] = useState(defaultValue.name || '')
  let [email, setEmail] = useState(defaultValue.email || '')

  return {
    error: name === '' ? 'Please enter a name' : undefined,
    name: {
      value: name,
      onChange: e => setName(e.target.value),
    },
    email: {
      value: email,
      onChange: e => setEmail(e.target.value),
    },
  }
}

This hook works fine when it is used inside a form component -- let's call it <ContactForm>. But say that your requirements change and you now need to be able to embed an arbitrary number of contacts in the form, and thus want to lift the state up into a <ContactList> component so that you can access all the contact state at once (see demo) -- there's no way to reuse the useContactModel() hook within <ContactList>, because it would need to be called within a loop.

function ContactList() {
  let contactIds = ['a', 'b', 'c']
  let contactModels = contactIds.map(useContactModel)

  return contactIds.map((id, i) =>
        <Contact
          key={id}
          model={contactModels[i]}
        />
  )
}

The contact form example feels a little contrived, but this scenario of wanting to store some "model" an arbitrary number of times comes up relatively often when building forms.

Proposed solution

I've taken a cursory look at the source but I really don't understand it enough to know if this could even work, but with that said...

Would it be possible to somehow assign keys to hooks, just as you can with components, and use that extra information to reconcile hooks called in conditionals/loops? For example, a hypothetical withKey function could accept a key and a callback, and allow for hooks to be called within the callback as if they were being called in a child component -- but returning the result of the callback instead of rendering elements.

let contactIds = ['a', 'b', 'c']
let contactModels = contactIds.map(id => 
  withKey(id, () => useContactModel())
)

Not sure if this is possible or even particularly desirable given the potential performance problems of the enormous hooks it would enable. It'd certainly make it easier to lift state up the tree, though.

gaearon commented 5 years ago

To be clear, it’s a very intentional limitation that we don’t support that. So don’t get too excited +1’ing this. :-) But I asked James to file this issue so we can take a closer look at this use case and see if there is an idiomatic solution.

gaearon commented 5 years ago

If you’re lifting state up, why do you want to keep it isolated? Hooks are for isolated state. But presumably, if you lift it, you want those states to “know” about each other and potentially affect each other. Or at least, it’s a possible next requirement. At that point, why not have a reducer managing it?

jamesknelson commented 5 years ago

I'm actually fine with the hooks not knowing about each other, just as child components don't know about their siblings. It's more that I want to treat them as "renderless components" that can be composed together to create bigger hooks/components.

Continuing with the form models example, reducers only allow you to keep state, but you may want more than just state:

Here's another example:

let userModel = useCombinedModels({
  // useModel() stores a field's state and any errors
  email: useModel({
    schema: Yup.string().required(),
    // call the server and check if the email is available
    validate: async (email) => checkIfEmailIsAvailable(),
  }),
  firstName: useModel({
    schema: Yup.string().required()
  }),
  lastName: useModel({
    schema: Yup.string(),
  }),
})

userModel.errors // holds combined errors of child models
userModel.value // holds a combined value that can be submitted to an API, etc.
userModel.children // holds individual models, that can be passed to form elements

The above should work with what we have now. However, There's no way to use hooks to represent an arbitrarily sized list of form elements, e.g.

let userListModel = useListModel(id => useUserModel())

userListModel.append({ defaultValue: ... }) // add a new form to the list
userListModel.value // holds an array of all child values
userListModel.children // holds an array of the individual contact models

It would certainly be possible to implement something like useUserListModel() using reducers and side effects, but it'd require a lot more thought than just throwing it together from useModel(), useCombinedModel() and useListModel().

The vibe I'm getting though is that hooks aren't really designed for this. Perhaps the best approach is to build the model using an external library, and just subscribe to it using context and hooks?

scottmas commented 5 years ago

It seems to me James that while you're okay with the children not knowing about each other, the top level component needs to know about all the children in order to perform some holistic validation, etcetera. Given that, the following is the best I could come up with as an idiomatic way of representing useUserListModel. It's certainly not as pretty as what you put together in your proposal, but it is very explicit and obvious where the state lives.

(Note I haven't tested this code and am still a rookie at hooks...)

function validateField(field, fieldValue) {
  //calculate validity and return error...
  return null;
}

function validateAllModelsHolistically(userModels) {
  //calculate validity and return error...
  return null;
}

function useUserListModel(users) {
  const initUserModels = users.reduce((acc, { id, name, email }) => {
    acc[id] = {
      name: {
        value: name,
        dirty: false,
        error: validate("name", email)
      },
      email: {
        value: email,
        dirty: false,
        error: validate("email", email)
      }
    };
    return acc;
  }, {});

  let [userModels, setUserModels] = useState(initUserModels);

  function onChangeHandler(userId, field, e) {
    setUserModels({
      ...userModels,
      [userId]: {
        ...userModels[id],
        [field]: {
          value: e.target.value,
          dirty: true,
          error: validateField(field, e.target.value)
        }
      }
    });
  }

  //Probably better ways to add the change handlers to the return value...
  const userModelsWithChangeHandlers = _.cloneDeep(userModels);
  Object.keys(userModelsWithChangeHandlers).forEach(userId => {
    userModels[userId].name.onChange = onChangeHandler.bind(null, userId, "name");
    userModels[userId].email.onChange = onChangeHandler.bind(null, userId, "email");
  });

  return {
    models: userModelsWithChangeHandlers,
    error: validateAllModelsHolistically(userModels)
  };
}

function ContactList() {
  const [userModels] = useUserListModel([
    { id: "a", name: "Alex", email: "alex@email.com" },
    { id: "a", name: "Bob", email: "bob@email.com" },
    { id: "a", name: "Charles", email: "charles@email.com" }
  ]);

  return (
    <>
      {Object.values(userModels.models).map(({ name, email }) => (
        <>
          <input value={name.value} onChange={name.onChange} />
          {name.dirty && !!name.error && <div>Error: {name.error}</div>}
          <input value={email.value} onChange={email.onChange} />
          {email.dirty && !!email.error && <div>Error: {email.error}</div>}
        </>
      ))}
      {userModels.error && <div>Holistic error: {userModels.error}</div>}
    </>
  );
}

tldr: You can use a whole lot of destructuring and cloning to create a single monolithic state object which contains all the different user models you care about. Plus by embedding the change handlers in the custom hook return value, you can have nice clean outward facing API that abstracts away the complex and monolithic structure of the state object.

jamesknelson commented 5 years ago

@scottmas Thanks for putting this together -- your code definitely makes sense as a way to implement my example with what React has right now.

The thing I was hoping to do was use hooks as a way to create composable models, but I'm coming around to the realization that they're really not designed for it. For truly composable form/data models, I think an external tool is probably necessary.

I'm going to close this for now, as from what @gaearon has said, keys for hooks aren't really something that's going to happen, and I think I understand why now.

scottmas commented 5 years ago

gaeron didn't really explain in depth why they aren't going to happen. Could you elaborate? What would be so awful about having keyed hooks? I do see how in some respects they could lead to more confusion over what component the state actually lives inside of, but they would be darn convenient.

jamesknelson commented 5 years ago

While not about keyed hooks in particular, I've tried to write down my understanding of why hooks aren't meant for this here: https://frontarm.com/james-k-nelson/react-hooks-intuition/

If this doesn't help, let me know what's missing and I'll see if I can improve the article.