final-form / react-final-form-arrays

A component for rendering and editing arrays 🏁 React Final Form
MIT License
205 stars 70 forks source link

Calling .push() on click can result in too many fields added #68

Open papermana opened 5 years ago

papermana commented 5 years ago

Are you submitting a bug report or a feature request?

This is a bug report.

What is the current behavior?

I cannot limit how many entries the user can push into an array. For instance, adding a conditional clause to a button's onClick handler doesn't always work. The problem appears when there is a high CPU utilization and several clicks can be made before a render.

I created a sandbox that shows off the problem. However, to actually see the issue you would probably need to use CPU throttling. Also, you may need to click like crazy. 😄

What is the expected behavior?

There should be a way to consistently limit how many fields can be added.

Sandbox Link

https://codesandbox.io/s/2zjl0w9l3n

What's your environment?

Just the "standard" one, like in the sandbox.

Other information

I assume the cause of this problem is that .push() is asynchronous. So multiple calls will get queued and, on a system with limited CPU power, may get executed before the next render gets finished. And the value of fields.length is only updated after render.

I'm not sure what the solution could be. Maybe provide something like an .update() method, which could be used like so:

onClick={() => fields.update(fields => {
  if (fields.length < 5) {
    fields.push('foo');
  }
})}

I can see that this issue is probably very niche. But it makes me uneasy that my users might get invalid UI (and/or any bugs that might come from e.g. sending too many fields to an API) just because they're installing an update on their computer at the same time or something.

It's also really not straightforward to get around this. For instance, the API doesn't provide a .slice() method for when we want to display only X entries.

TrejGun commented 5 years ago

I also have problem with push my example used to work in v2 https://codesandbox.io/s/react-final-form-field-arrays-dehxx but after update to v3 it shows only 1 field instead of 5

jvke commented 4 years ago

@TrejGun This is fixed by changing the dependency of your useEffect fn to just fields: https://codesandbox.io/s/react-final-form-field-arrays-onrfv

TrejGun commented 4 years ago

do you mean

  useEffect(() => {
    if (fields.length < 5) {
      fields.push();
    }
  }, fields);

thsi results in error from react

React Hook useEffect was passed a dependency list that is not an array literal. This means we can't statically verify whether you've passed the correct dependencies. (react-hooks/exhaustive-deps)
eslint

because fields are not an array but array-like object

jvke commented 4 years ago

@TrejGun That error is because the useEffect dependency argument expects an array, ie.

  useEffect(() => {
    if (fields.length < 5) {
      fields.push();
    }
  }, [fields]);
Hyokune commented 4 years ago

When I use the useEffect function, it seems to push two into the array rather than one. Is there a way around this? componentDidMount doesn't seem to work as well.