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

Having this.state in the constructor and this.setState everywhere else is violating the "Uniform access principle" #7212

Closed
santiagobasulto opened this issue Jul 7, 2016 · 6 comments

Comments

@santiagobasulto
Copy link

ES6 style of creating components in React is clearly violating the Uniform access principle. It's a general source of confusion (and bugs) to be able to set state in two different ways:

  • You MUST do this.state = {} if you're in the constructor
  • You MUST do this.setState() everywhere else.

I believe it'd be much better to have just 1 way to set state in general, and it should be universal. If we're already accessing state through this.state, and setting state in the constructor with this.state, why not do it everywhere else? The behavior of this.setState could be easily replicated with ES6 Proxies.

@gaearon
Copy link
Collaborator

gaearon commented Jul 7, 2016

We definitely won’t be using ES proxies in production builds because they are a new feature and not supported in older browsers.

I generally agree this is confusing. The justification I heard from @sebmarkbage is that while constructor runs, the instance doesn’t exist in ReactInstanceMap yet (which makes sense because the constructor has not finished). So we can’t schedule an update on it like we normally do in ReactUpdates.

I don’t, however, quite see why we couldn’t make setState write to this.state until it’s in the map. Maybe you could give it a try, and we could discuss a PR that does this. (I don’t fully understand the implications so no promises about merging this, but it could be a starting point for a discussion.)

I think another intention was that you’d never write constructor logic at all, and instead you would use property initializer syntax (class X extends Component { state = { counter: 0 }; ... }). However this isn’t part of the language yet, and we don’t know if it makes it there.

@aweary
Copy link
Contributor

aweary commented Jul 7, 2016

Do you feel that getInitialState also violates the UAP? Because this.state = { ... } is essentially the equivalent in ES6 classes. Initializing state as a property on the class seems much clearer than having to call setState in the constructor.

It's a general source of confusion (and bugs) to be able to set state in two different ways:

Maybe if you were allowed to update state with this.state = ... it would be confusing, but IMO there is a pretty clear distinction between state initialization and state updates. You can't update state with assignment to this.state and you can't initialize state with this.setState. They have distinct purposes.

@sebmarkbage
Copy link
Collaborator

Technically, the best way to "setState" is initializer state = 0 and then this.setState(state => state.counter + 1); because setState should just add a reducer to the queue. Now you have to conditionally pass undefined as the initial state there. "setState" is a misnomer because it is really "enqueueStateTransition" but there's nothing to transition from initially. :)

@gaearon That'd be the only case where this.setState would be synchronous (since we want always-batched).

You can also store it in a side-table. You also need to store the second argument to setState in a side-table (or kill that feature). Storing a global side-table wasn't good because you can call out to other component's setState so it'd need to be a weakmap.

The side-table thing violates the composition pattern:

class Foo extends Component {
  constructor(props, context) {
     this._wrapped = new OtherComponent(props, context, customUpdater);
  }
  ...
}
class Bar extends OtherComponent {
  constructor() {
    return {
      __proto__: new.target.prototype,
      _wrappedAllocation: super()
    };
  }
}

Who know what other weird class semantics are not possible if you break the rules. Classes are just weird.

@bvella
Copy link

bvella commented Jul 7, 2016

Could be a stupid suggestion, but this could possibly be mitigated by:

  • adding a _constructing property in ReactComponent constructor
  • update ReactComponent setState() to check this property and set this.state if constructing
  • delete the property in ReactCompositeComponent _constructComponentWithoutOwner() (which I think is the only place a component is constructed)

This would allow the use of this.setState() throughout, and in the constructor would be equivalent to this.state = {}

@danielearwicker
Copy link

I think they are different operations. In constructor we are specifying a complete object. In setStatewe are specifying a partial object.

Using the terminology of this potential TS feature: microsoft/TypeScript#4889 - in those terms, if a component's state is of type MyState, then state's type is MyState, whereas setState would take partial MyState.

@gaearon
Copy link
Collaborator

gaearon commented Oct 27, 2016

We likely won't do this.

If property initializers get into the language, you'd just state = ... as a property rather than use constructor so there is no problem.

As for componentWillMount we don't think it's very useful anyway when you have a constructor.

We will likely revisit all lifecycles after shipping Fiber (#7678) and we'll keep this in mind when designing the new ones.

@gaearon gaearon closed this as completed Oct 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants