Improve Immutability

[tannerlinsley]

Aside from everything functioning great, performance and component lifecycle could be slightly improved by enforcing immutability throughout a few places. I'm open to ideas.

[dcolucci]

Not directly related to immutability, but a popular way to improve performance / UX of React applications is to optimize rendering using the shouldComponentUpdate lifecycle method. Right now, every time the top-level form sets a new state or receives new props, every component in its tree is re-rendering. This means all render logic is called and then React runs its internal virtual DOM diff to see what, if any, browser DOM operations it needs to make. There are many circumstances when you actually do need to re-render based on changes to props or state, but maybe there are some in which you do not want a re-render to trigger. For instance, maybe the form does not need to re-render on changes related to touched? If there are no UI changes needed, you probably don't need to re-render.

You could also consider similarly optimizing your "helper" components (FormError, etc). Right now they are implemented as functional stateless components, but each of them is re-rendering whenever the parent form re-renders. It is not a huge lift since these are mostly simple components but it's something to consider. And if the user passes some massively complex component to your Form wrapper, that too will re-render (though that component itself could implement this optimization).

I also notice React has introduced a PureComponent which has a shallow state / props comparison built into shouldComponentUpdate to provide some out-of-box performance optimization. I haven't yet used this feature myself though.

EDIT: thinking about it more, actually this concern could be largely obviated by the user just including shouldComponentUpdate optimization on the wrapped component. So maybe this isn't high priority as an enhancement.

[tannerlinsley]

That's a great start for sure. I've never used PurComponent's either, and judging by the reliance on the deeper data structures, it might produce some false negatives if we did use it.

I know this is one of the tipping points that redux-form had where they were supplying many props (much like react-form does) to the child component that renders the form. Because of this pattern, the child component had to update even if only one field in the values object changed. I imagine this is why they moved to a 100% opt-in value subscription and field wrapper mentality.

It might be challenging to find the middle ground, but right now I feel that the easy access to the state values via props is more valuable in comparison to the edge case performance benefits that a value subscription pattern gives.

What do you think?

[dcolucci]

I think you are right, it's probably not worth it at this point. The Form's own render logic is actually pretty light, it's more a concern of how complex the downstream components are, which can be more granularly controlled if they implement shouldComponentUpdate.

What might be cool though to try is taking some hard measurements, and then you can see how performance changes as the wrapped component varies in complexity, and also sanity-check that future enhancements to your code don't tank performance.

You could utilize benchmark.js, enzyme, and jsdom to create a suite of tests in which the React components mount to a virtual DOM. I haven't actually ever set this up successfully but I imagine someone has. I might try playing with it later on.

[tannerlinsley]

One compromise I am imagining is possibly putting all of the formstate props behind their own subscriber components permanently. It would be similar in spirit to redux-form's approach, but instead of needing to write lengthy selectors, you would be referencing values much closer to home.

imagine:

Form()(({values}) => {
  return (
    <form>
      <FormInput field='something'/>

      {values.friends.map((friends, i) => (
        <div key={i}>
          <Text
            field={['friends', i, 'name']}
            placeholder='Friend Name'
          />
        </div>
      ))}

    </form>
  )
})

// becomes

Form()(() => {
  return (
    <form>
      <FormInput field='something'/>

      <FormValue field='friends'>
        {(friends) => (
          friends.map((friends, i) => (
            <div key={i}>
              <Text
                field={['friends', i, 'name']}
                placeholder='Friend Name'
              />
            </div>
          ))
        })}
      </FormValue>

    </form>
  )
})

[tannerlinsley]

That would allow the main form component to be treated as pure, unless it's own props change :)

[tannerlinsley]

I'm trying to find any other props the form passes down that would need to be placed behind a subscriber component if we took this approach...

[tannerlinsley]

This might take some more thinking and discover as the library matures. For now, everything works and performance is on par with what is expected from React. Closing until it becomes a real issue.