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

Introducing ReactComponentBase base class #2805

Merged
merged 1 commit into from
Jan 13, 2015

Conversation

sebmarkbage
Copy link
Collaborator

This is the base class that will be used by ES6 classes.

I'm only moving setState and forceUpdate to this base class and the other
functions are disabled for modern classes as we're intending to deprecate
them. The base classes only have getters that warn if accessed. It's as if
they didn't exist.

ReactClass now extends ReactComponentBase but also adds the deprecated
methods. They are not yet fully deprecated on the ReactClass API.

I added some extra tests to composite component which we weren't testing
to avoid regressions.

I also added some test for ES6 classes. These are not testing the new
state initialization process. That's coming in a follow up.

@glenjamin
Copy link
Contributor

With replaceState being deprecated, is it intentional to prohibit state from being something other than a plain JS object?

I've seen a bunch of examples where an Immutable.Map is used directly, but I don't think that works if only setState exists (and merges).

@sebmarkbage
Copy link
Collaborator Author

The idea is to make state a record-like object with fixed keys. E.g. you should specify all keys in the initial state and never add extra keys. It ensures type stability both for type systems and VMs. It also ensures that you explicitly define an initial and descriptive value.

We could potentially support arbitrary state objects, but props is also always an object (could also become a record). The idea is that these objects act as named arguments. That way it's always easy to add and remove new arguments. I think it's valuable to keep that consistency through out components.

You can always put anything else one level below it. Immutable.Map isn't ideal as the primary state object since it's dynamic. Something like Immutable.Record is closer to what I had in mind. We could potentially support Immutable.Record natively.

These changes are not set in stone. The reason I'm not supporting them yet is to make this new API more restrictive to ensure some kind of insurance of future compatibility. Please, feel free to describe your use case with some real world examples.

We're playing around with updaters returning state in which case it would probably auto-merge: https://github.com/reactjs/react-future/blob/master/07%20-%20Returning%20State/01%20-%20Stateful%20Functions.js#L17

@glenjamin
Copy link
Contributor

I think if there's an improved API for accessing _pendingState, or a way to define custom setState merge behaviour, no functionality is lost.

If you only have setState, and want to use some custom flavour of collection in a similar way to how setState works (different handlers updating different keys), then it's quite tricky to ensure clean updates.

I've seen code like this in a few demos:

this.replaceState((this._pendingState || this.state).merge({some: changes}));

Supporting Record might be neat, but if going down that route I think some sort of protocol that other libs can implement would be preferrable.

@sebmarkbage
Copy link
Collaborator Author

Let's move the replaceState discussion to #2843 since it's not directly related to this pull request.

instance.setState({ value: 1 });
}).not.toThrow();

React.unmountComponentAtNode(container);
Copy link
Member

Choose a reason for hiding this comment

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

We don't actually unmount in most of our tests. I think you could skip this, as well as appending container to the DOM (non of our tests do this, just having the node is good enough)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test is testing componentWillUnmount so I need to unmount it to trigger the expect in the test case.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, I missed that.

This is the base class that will be used by ES6 classes.

I'm only moving setState and forceUpdate to this base class and the other
functions are disabled for modern classes as we're intending to deprecate
them. The base classes only have getters that warn if accessed. It's as if
they didn't exist.

ReactClass now extends ReactComponentBase but also adds the deprecated
methods. They are not yet fully deprecated on the ReactClass API.

I added some extra tests to composite component which we weren't testing
to avoid regressions.

I also added some test for ES6 classes. These are not testing the new
state initialization process. That's coming in a follow up.
sebmarkbage added a commit that referenced this pull request Jan 13, 2015
Introducing ReactComponentBase base class
@sebmarkbage sebmarkbage merged commit d138f9a into facebook:master Jan 13, 2015
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.

3 participants