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

[v6] Why do all fields re-render when a single field changes? #1405

Closed
andrewmclagan opened this Issue Jul 27, 2016 · 20 comments

Comments

@andrewmclagan

andrewmclagan commented Jul 27, 2016

Im sure this has been answered.

Tracing component updates in the console shows on every: keypress and update event ALL fields are re-rendered. This seems like a performance bottle neck with large forms?

Already when I try to type fast in a <textarea /> its slow, much slower then non-redux-form.

@xiaofan2406

This comment has been minimized.

xiaofan2406 commented Jul 27, 2016

Any props or state changes in a component will cause the whole component to re-render. The 'Trace component updates' shows what components are being re-render by React, but it doesn't mean that the DOM tree for the component is completely redrawn. React is effect enough to re-draw the necessary dom tree.

@cristian-sima

This comment has been minimized.

cristian-sima commented Jul 27, 2016

I think one of the ways to see if the component really updated on dom is to use the Chrome Paint Flasing from Rendering section.

image

@richardscarrott

This comment has been minimized.

richardscarrott commented Jul 27, 2016

@andrewmclagan I believe that v5 re-rendered all fields because it's the top level Form component which is connect()ed to the store so whenever the store changes, i.e. whenever you change a value in the form it re-renders. This makes for a great programming model in true Flux / React style however I believe many people ran into performance issues as unfortunately whilst JS is fast, it's not that fast.

I think this is why in v6 each individual <Field /> is connect()ed to the store so only specific fields need to update...saying that, store.dispatch would still have to trigger change on each of the connected Fields so unless there's some fancy optimisation in place I can't imagine it being that much faster... actually, @erikras I'd be really interested to hear more about the performance characteristics between v5 and v6?

@erikras

This comment has been minimized.

Owner

erikras commented Jul 28, 2016

v6 should not rerender every input on every keypress. One confusing aspect is a potential confusion between updates and renders, which are not the same. If you have an example that can be shared where your entire form is being slowed down with each keypress, by all means share it.

@richardscarrott

This comment has been minimized.

richardscarrott commented Jul 28, 2016

@erikras Would I be right in saying that in v6 every <Field /> is notified of a change to the store on every keypress (even if connect determines it doesn't necessarily need to render via shouldComponentUpdate)?

@erikras

This comment has been minimized.

Owner

erikras commented Jul 28, 2016

@richardscarrott Yes, just like every connect()ed component in your app.

@richardscarrott

This comment has been minimized.

richardscarrott commented Jul 28, 2016

@erikras so if you had 300 <Field /> components in your form (not saying I have this use case but just for stress testing purposes) it'd still be similarly expensive as it was in v5 when the whole form was rendered or have you found it to in fact be faster?

@erikras

This comment has been minimized.

Owner

erikras commented Jul 28, 2016

Yes. The slowness is in the render, which consists of React reconciliation (its magic virtual DOM stuff) and the actual DOM updating. Running a deepEqual() compare of two js objects pales in comparison.

@andrewmclagan

This comment has been minimized.

andrewmclagan commented Jul 28, 2016

On some form components we have developed specifically for redux-form we have HUGE performance benefits front only passing 'props.onChange' a new value when our underlying input 'input.onBlur' is called.

Maybe... Just maybe we could default to blur updates? And users can opt in to onChange updates?

@andrewmclagan

This comment has been minimized.

andrewmclagan commented Jul 28, 2016

Have not thought it out allot... I'm sure there are caveats to this approach. Although it does have massive benefits and I can't see a reason store needs updating on every change event

@schwermer

This comment has been minimized.

schwermer commented Jul 29, 2016

We also have seen a huge performance benefit from ignoring onChange events and only updating the redux-form state onBlur for forms with large FieldArrays. The most obvious downside to this approach is that normalization does not occur while the user is typing.

I wonder if an option in the reduxForm config or the Field component to only update onBlur would be worth including with this package? I would be happy to submit a PR if there is interest

@richardscarrott

This comment has been minimized.

richardscarrott commented Jul 29, 2016

@erikras I spent some time benchmarking the performance of v5 and v6 with various size forms and I've found myself able to tune v5 better than v6.

The reason I've been able to get v5 running faster is because I'm able to prune large parts of the component tree during a re-render by separating my form into multiple fieldsets. This isn't possible in v6 because the heavy lifting has in effect been taken away from React and given to Redux's event publisher which isn't designed to be optimised in the same way React render is.

I've pushed up my demo along with the HAR files demonstrating the difference in frame rate whilst typing in a form with 40 and another with 400 inputs. If you have time it'd be great if you could review the code to determine if you can squeeze out any more perf on either versions?

v5
https://github.com/richardscarrott/redux-form-v5-vs-redux-form-v6/blob/redux-form-v5/src/components/AddressForm/AddressForm.js

v6
https://github.com/richardscarrott/redux-form-v5-vs-redux-form-v6/blob/redux-form-v6/src/components/AddressForm/AddressForm.js

@richardscarrott

This comment has been minimized.

richardscarrott commented Aug 9, 2016

@erikras did you get a chance to look over those benchmarks at all? The main reason I ask is because I can see you're looking to land v6 very soon and I'm concerned the really nice v5 API is going to be dropped even if there aren't performance benefits in v6...

Incidentally, I wrote a blog post describing the performance optimisation I was able to make in v5 which wasn't possible in v6 -- https://richardscarrott.co.uk/blog/post/rendering-big-lists-in-react/

@timhwang21

This comment has been minimized.

Contributor

timhwang21 commented Sep 7, 2016

Thought I would bump this to the top. I'm having the same issue with all fields re-rendering (I specifically checked the render method of <Field> and confirmed that yes, this is what's happening).

@schwermer , would it be possible to share your solution of only updating onBlur? I'd love to take a look.

Thanks!

@smirea

This comment has been minimized.

Contributor

smirea commented Sep 8, 2016

the onBlur workaround simply implies removing the onChange listener from the input.
The main downside is that normalization and validation don't occur on keypress.
Another key factor is that if you use custom plugins for reducers, you have to listen for the redux-form/BLUR action to check when a field is changed (since the CHANGE event is never fired)

aka, for the following example:

<Field name='foo' component={renderTextField} />

normal way (bind all listeners: change, blur, focus, etc), that causes a CHANGE action to be dispatched on every keypress

const renderTextField = ({input, meta, ...custom}) => <input type='text' {...input} {...custom} />

only using onBlur:

const renderTextField = ({input: {onChange, ...input}, meta, ...custom}) => 
    <input type='text' {...input} {...custom} />

This prevents the onChange events from ever firing

@timhwang21

This comment has been minimized.

Contributor

timhwang21 commented Sep 8, 2016

Perfect! Will play with this. Thanks!

@bertjwregeer

This comment has been minimized.

bertjwregeer commented Oct 29, 2016

@smirea If you don't call onChange then value is never updated, so you end up with a form field that won't let you input any data (since it is not round-tripping to React for a DOM refresh).

You'd have to make the form component stateless by removing value from the props passed to <input> but then you can't have redux form provide an initial value...

I ended up with this, and it seems to work (probably some corner case I am missing somewhere...)

export class inputOnBlur extends Component {
    state = {
        value: '',
    }

    componentWillMount = () => {
        const { input: { value } } = this.props;

        this.setState({value: value});
    }

    componentWillReceiveProps = (nextProps) => {
        const { input: { value } } = nextProps;

        this.setState({value: value});
    }

    onChange = (e) => {
        this.setState({value: e.target.value});
    }

    render() {
        const { input, type, placeholder } = this.props;

        delete input.onChange;
        delete input.value;

        input.value = this.state.value;
        input.onChange = this.onChange;

        return (
                   <input type={type} placeholder={placeholder} {...input} />
               );
    }
}
@semy

This comment has been minimized.

semy commented Dec 29, 2016

Are there any news about this issue?

@erikras

This comment has been minimized.

Owner

erikras commented Dec 30, 2016

Two things:

  1. I'm not really sure what to tell you. While I seriously doubt it, I cannot verify the claim that all the fields rerendered on every change back in July and August, during the rocky v6-alpha period. I can, however, verify that it is not the case now. See the commit above. ☝️
  2. Since this ticket was introduced, there is a new component called Fields that will more or less return sections of your form to v5 functionality, where you get one component that is given all the metadata and props for many fields. So if you wanted to do optimization like @richardscarrott suggests above, you could potentially use that.

P.S. Your fields will rerender every time if you are creating your component function every time. Don't do that.

<Field name="foo" component={props => <input {...props.input}/>}/> //  ❌ WRONG
const renderInput = props => <input {...props.input}/>
...
<Field name="foo" component={renderInput}/> //  ✅ RIGHT
@lock

This comment has been minimized.

lock bot commented Jun 2, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.