gtournie / redux-form-validators

redux-form-validators
152 stars 22 forks source link

Provide memoized `every` and `some` combinators #56

Closed futpib closed 5 years ago

futpib commented 5 years ago

Using arrays to combine validators may introduce unnecessary updates (as a new array is created on each render).

<Field
  validate={[required(), email()]}
/>

If redux-form-validators provided an every combinator, which would memoize similarly to other validators, this could be avoided.

<Field
  validate={every(required(), email())}
/>

While we at it, why not add some with the opposite meaning of "at least one validator should pass".

gtournie commented 5 years ago

I don't feel like it's very relevant to create an every function for this purpose. First, that would introduce a double memoization process which would be cumbersome to manage. And second, I'm pretty sure such a func would have a bigger impact in terms of performance and memory.

Now if you're really concerned about the memory footprint of creating a new array (we're talking about a micro-optimization here) on each render, you should define your validators outside of the render method. And while we're at it, you can also deactivate the memoize mechanism :

Validators.defaultOptions.memoize = false
const validations = {
  username: [required(), length({ min: 8 })],
  password: [required()]
}
...
<Field validate={validations.username} .../>

And if you really like the idea of having methods like every/some, feel free to implement them on your end (and eventually to share them here). It doesn't necessarily have to be part of this lib :)

futpib commented 5 years ago

I already had to implement every, not due to memory concern, but, as I said, to prevent unnecessary re-renders, while keeping validators inline for readability.

gtournie commented 5 years ago

According to the doc, React will only re-render a Component in these situations:

  1. when your component get mounted initially
  2. when state got changed using this.setState()
  3. when your component receives new props
  4. when this.forceUpdate() get called.
  5. when context gets changed

So there is absolutely no reason why [required(), email()] would call render more than every(required(), email())

And regarding your memory concerns, unless you call render a billion times, this is really a micro-optimization. I invite you to copy and paste this snippet in Chrome dev tools and check the memory footprint:

(function memoryTest() {
  let obj1 = {},
    obj2 = {},
    a
  console.time('t')
  // 1 billion times
  for (var i = 0; i < 1000000000; ++i) {
    // storing an object or a function is the same. Only the reference is stored in memory
    a = [obj1, obj2]
  }
  console.timeEnd('t')
})()

If you have a use-case/an example where you can clearly show me a memory issue with the array approach, I'll be happy to reconsider adding combinators ;)

futpib commented 5 years ago

The re-render I'm talking about would fall under "when your component receives new props".

Here is a demo: https://codesandbox.io/s/nk31k3rpx4. Notice how a Field with an array of validators is re-rendered, but a field with one validator is not. This is because props are compared shallowly to determine if a component should update, and a new array is created each time (and it is not ===-equal to a previous array).

I originally thought these updates also propagated down to the component wrapped in Field, but I could not reproduce this now (neither in the codesandbox, nor in a project of mine where I got update loops and thought I had to memoize the array validators to prevent them). So maybe this still is not worth it, as updates stop at the level of Field. But the update is still there :wink:

I do not have memory concerns here.

gtournie commented 5 years ago

Ha. I see what you mean. That makes sense now.

Now I feel like it should mostly be a React or Redux-form optimization.

Also, if people really want to optimize the rendering of their component, they can either :

Thanks anyway for pointing this out! :)

gtournie commented 5 years ago

Hey @futpib. Only stupid people never change their mind!

I realized recently that redux-form-validators was also compatible with react-final-form. Thing is, react-final-form doesn't accept arrays of validators... So I decided to kill 2 birds with one stone and implemented combine.

I gave a try to every/some, but some was really tricky. I had this example about a field with a landline phone validation or a mobile phone validation. Perfect for some right? Especially if you already wrote those 2 validators. The problem is, what error do you return in that case? You can't just return one of them, it wouldn't be accurate. And you can't also return both of them, you would have to somewhat link them together with a " OR " or something... which is not really elegant and would also mess with internationalization...