Skip to content
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

Don't bail out on referential equality of Consumer's props.children function #12470

Merged

Conversation

iamdustan
Copy link
Contributor

@iamdustan iamdustan commented Mar 28, 2018

I was playing around with the new context API and ran into some unexpected bail outs. The exact scenario was something like the following:

class React extends React.Component {
  state = { name: '' };
  onChange = (event) => {
    const { name, value } = event.currentTarget;
    this.setState({[name]: value});
  };
  renderChildren = (context) => {
    return (
      <form action={context.action}>
        <input value={this.state.value} onChange={this.onChange} name="name" />
      </form>
    );
  };

  render() {
    return <Consumer>{this.renderChildren}</Consumer>;
  }
}

The test case here is similar but without any DOM dependencies. I would expect this to “just work” but alas. My only thoughts for “solving” this is checking if the function exists on the instance before optimistically bailing out and assuming it is pure.

@gaearon
Copy link
Collaborator

gaearon commented Mar 28, 2018

I’m struggling to understand what you’re trying to do a bit.

Do you expect React to propagate the context update even though the context value prop has not changed? Is there any reason you can’t put the thing you want to pass to the consumer into the context value?

Why are both provider and consumer in one component? Is this representative of the real code you’re writing?

Can you provide a real world example? I’m not sure I see how the code in your test case relates to the code in the description.


renderConsumer = (context) => {
ReactNoop.yield('App#renderConsumer');
return <span prop={this.state.value} />;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon The issue is not so much about context propagating, but that this instance method that reads from state to be rendered and updated when setState is called.

Copy link
Contributor Author

@iamdustan iamdustan Mar 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following is pretty close to actual code that experienced this.

// @flow

import * as React from 'react';
import { login } from 'api';
import { BetaFeatureConsumer } from 'BetaFeatures';
import { SingleSignOn } from 'SingleSignOn';

type P = {};
type S = {
  username: string,
  password: string,
  loading: boolean,
  errors: *,
};

export default class Login extends React.Component<P, S> {
  state = {
    username: '',
    password: '',
    loading: false,
    errors: null,
  };

  onChange = (event: SyntheticEvent<HTMLInputElement>) => {
    const { name, value } = event.currentTarget;
    this.setState({ [name]: value });
  };

  onSubmit = (event: SyntheticEvent<*>) => {
    event.preventDefault();
    this.setState({ loading: true, errors: null });
    login(this.state.username, this.state.password).then(data => {
      this.setState({ loading: false });
      if (data.hasError) {
        this.setState({ errors: { ...this.state.errors, ...data.errors } });
      }
    });
  };

  _render = v => {
    return (
      <form onSubmit={this.onSubmit}>
        <input
          label="Username"
          name="username"
          onChange={this.onChange}
          value={this.state.username}
        />
        <input
          label="Password"
          name="password"
          onChange={this.onChange}
          value={this.state.password}
          type="password"
        />
        <button type="submit" disabled={this.state.loading}>
          Submit
        </button>
        {v.singleSignOn && <SingleSignOn onComplete={this.onSubmit} />}
      </form>
    );
  };

  render() {
    return (
      <BetaFeatureConsumer>
        {this._render}
      </BetaFeatureConsumer>
    );
  }
}

@iamdustan iamdustan changed the title Test case for React Context bailing out unexpectedly React Context bails out when this.state is updated unexpectedly Mar 28, 2018
@gaearon
Copy link
Collaborator

gaearon commented Mar 28, 2018

I see. Here's an example:

const Ctx = React.createContext();

class Thing extends React.Component {
  state = {counter: 0};

  renderValue = (value) => {
    return <h1>Hello, {value}, {this.state.counter}</h1>;
  }

  render() {
    return (
      <React.Fragment>
        <Ctx.Consumer>
          {this.renderValue}
          {/* it would work with v => this.renderValue(v) */}
        </Ctx.Consumer>
        <button onClick={() =>
          this.setState(prevState => ({ counter: prevState.counter + 1 }))
        }>
          +
        </button>
      </React.Fragment>
    );
  }
}

ReactDOM.render(
  <Ctx.Provider value="hello"><Thing /></Ctx.Provider>,
  document.getElementById('container')
);

It's confusing that setState has no effect.

@iamdustan
Copy link
Contributor Author

Yes. That exactly.

@@ -995,8 +995,18 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
renderExpirationTime,
);
} else if (oldProps !== null && oldProps.children === newProps.children) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to fix it, it seems that we should remove this whole bailout branch.

@sebmarkbage
Copy link
Collaborator

Normally we bail out of children equality because it is very unusual to cache React elements but not impossible. E.g. this pattern would still fail to update:

class ... {
  ...
  consumer = <Ctx.Consumer>{value => <h1>Hello, {value}, {this.state.counter}</h1>}</Ctx.Consumer>;
  render() {
    return consumer;
  }
}

There is an argument to be made that since it is common to cache closures, we shouldn't apply the same bailout strategy for render props.

It's pretty sketchy though. I think in the future, we'll just want to move away from bound closures on instances and instead always provide new closures.

So the tradeoff is whether we want to optimize for future best practices or current. If we allow this pattern now, we can't really easily unallow it later.

@gaearon gaearon changed the title React Context bails out when this.state is updated unexpectedly Don't bail out if on equality of Consumer's props.children function Mar 29, 2018
@gaearon gaearon force-pushed the context-updates-on-instance-children branch from b6f1a2a to f7ce2f9 Compare March 29, 2018 17:15
@gaearon gaearon changed the title Don't bail out if on equality of Consumer's props.children function Don't bail out on referential equality of Consumer's props.children function Mar 29, 2018
}
// There is no bailout on `children` equality because we expect people
// to often pass a bound method as a child, but it may reference
// `this.state` (and thus may need to re-render on `setState`).
Copy link
Contributor

@bvaughn bvaughn Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The same is true for this.props)

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me.

@gaearon gaearon merged commit 15e3dff into facebook:master Mar 29, 2018
@iamdustan iamdustan deleted the context-updates-on-instance-children branch March 29, 2018 18:42
@iamdustan
Copy link
Contributor Author

thank you! 👍

rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
…unction (facebook#12470)

* Test case for React Context bailing out unexpectedly

* This is 💯% definitely not the correct fix at all

* Revert "This is 💯% definitely not the correct fix at all"

This reverts commit 8686c0f.

* Formatting + minor tweaks to the test

* Don't bail out on consumer child equality

* Tweak the comment

* Pretty lint

* Silly Dan
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants