kettanaito/react-advanced-form

Each Field triggers the render method of the Form Component on load

johnpreed opened this issue · 3 comments

Environment

  • react-advanaced-form: 1.6.7

What

Current behavior

On the initial page load of a form with fields, the render method of the form is triggered once for each field registration.

Expected behavior

Collect field registrations somehow and update the state in one go.

Why

I realize this is probably by design, because the field registration needs to update the state of the form to add the field to the state. However, the side effect of this is the render method being called for every single field in the form.

How

Steps to reproduce the issue (ideally, link to the repository/sandbox).

  1. Add a Form element with multiple fields.
  2. Add a sub component to the form that returns a list of fields.
  3. Add a console.log("sub component") inside the render method of the sub component.
  4. Note the number of times the console.log() line will be called will be a factor of the number of fields in your form.

Hi, @johnpreed.

That's indeed weird. Field registration events should be buffered when occurred under defined amount of time (50ms currently):

fromEvent(eventEmitter, 'fieldRegister')
.pipe(bufferTime(50))
.subscribe((pendingFields) => pendingFields.forEach(this.registerField))

What I should mention is that amount of state updates !== amount of re-renders. React re-renders components often, even without explicit state updates (nested children, for example). So I would paraphrase the issue as:

Check if field registration is buffered and produces 1 form state update even for multiple fields registering at the same time.

I am quite busy at the moment, may I kindly ask you to double check? I believe it should be sufficient to put some logging under this state update call:

this.setState({ fields: nextFields }, () => {

I was able to repro the behavior seen as far as the render() method on a subcomponent being called multiple times. The setState() is only being called for each field that is registered, but the child components are being rendered each time. Is expected behavior?

Screen Shot 2019-03-21 at 2 43 20 PM

Here was my setup to repro:
DebounceTime.jsx

import React from 'react'
import { FormProvider, Form } from 'react-advanced-form'
import { Input } from '@examples/fields'
import MyComponent from './MyComponent'

export default class DebounceTime extends React.Component {
  render() {
    return (
      <div>
        <h1>FormProvider</h1>
        <FormProvider>
          <Form>
            <Input
              name="fieldOne"
              label="Field with default debounce time"
              rule={/foo/}
            />
            <MyComponent label={"MyComponent1"} name={"test"} />
            <MyComponent label={"MyComponent2"} name={"test2"} />
            <MyComponent label={"MyComponent3"} name={"test3"} />
          </Form>
        </FormProvider>

        <FormProvider debounceTime={0}>
          <Form>
            <Input
              name="fieldTwo"
              label="Field with custom debounce time"
              rule={/foo/}
            />
          </Form>
        </FormProvider>
      </div>
    )
  }
}

MyComponent.jsx

import React from 'react'
import { Input } from '@examples/fields'

export default class DebounceTime extends React.Component {
  render() {
    console.log(`${this.props.label}`);
    return (
      <div>
        <Input
            name={this.props.name}
            label={this.props.label}
            rule={/foo/}
        />
      </div>
    )
  }
}

Yes, I believe this is the expected behavior. Field registration produces state update, which, in its turn, produces context update. This is how fields can render relevant state. Re-render of all form children occurs after form's state update, just as it would in any other React component.

It's a common context pattern, and I don't think there is much to go wrong on RAF side. In case of performance issues you can always use shouldComponentUpdate on your field components, but I wouldn't recommend that.

As I've said, the issue to verify is wether N amount of fields registered under 50ms trigger this.setState in their parent form just once. To assert that buffering works as expected.