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.

Sign up for GitHub

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

async-safe, static lifecycle hooks #6

Merged
merged 22 commits into from
Jan 19, 2018
+442 −0
Merged

async-safe, static lifecycle hooks #6

Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9818f57
Initial proposal for async-friendly, static lifecycle hooks
bvaughn Dec 8, 2017
839547f
Added a note about the prefix
bvaughn Dec 8, 2017
b7189cc
Made deprecation/rename clearer
bvaughn Dec 8, 2017
aad6845
Hopefully clarified the computeMemoizeData() placeholder example
bvaughn Dec 8, 2017
99988da
Renamed example method for improved clarity
bvaughn Dec 9, 2017
4a34d25
Added note about aEL being unsafe in cWM
bvaughn Dec 9, 2017
902d4b3
Fixed typo
bvaughn Dec 9, 2017
3df7ce6
Fixing typo
bvaughn Dec 9, 2017
1864c93
Removed Facebook-specific terminology
bvaughn Dec 9, 2017
428758b
Tweaked wording for clarity
bvaughn Dec 9, 2017
cfcb7e8
Reorganization (part 1, more to come)
bvaughn Dec 9, 2017
536084d
Added some more focused examples
bvaughn Dec 9, 2017
a1431a4
Added a comment about calling setState on a possibly unmounted component
bvaughn Dec 9, 2017
3c6132e
Cancel async request on unmount in example
bvaughn Dec 9, 2017
4425dbe
Tweaking examples based on Dan's feedback
bvaughn Dec 9, 2017
67272ce
Renamed deriveStateFromProps to getDerivedStateFromNextProps
bvaughn Dec 13, 2017
c7f6728
Renamed prefetch to optimisticallyPrepareToRender
bvaughn Dec 13, 2017
bb2d246
Added `render` to a list
bvaughn Dec 14, 2017
8618f70
Removed static optimisticallyPrepareToRender() in favor of render()
bvaughn Dec 19, 2017
8f6c20e
Renamed getDerivedStateFromNextProps to getDerivedStateFromProps
bvaughn Jan 18, 2018
e05e317
Updated when getDerivedStateFromProps is called and what its argument…
bvaughn Jan 18, 2018
7042a2a
Renamed unsafe_* to UNSAFE_*
bvaughn Jan 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Renamed getDerivedStateFromNextProps to getDerivedStateFromProps
@bvaughn
bvaughn committed Jan 18, 2018
commit 8f6c20edd98f16ecc7395b83332944d1757590a4
10 changes: 5 additions & 5 deletions text/0000-static-lifecycle-methods.md
View file
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@ At a high-level, I propose the following additions/changes to the component API.

```js
class ExampleComponent extends React.Component {
static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
static getDerivedStateFromProps(nextProps, prevProps, prevState) {
// Called before a mounted component receives new props.
// Return an object to update state in response to prop changes.
// Return null to indicate no change to state.
@@ -35,7 +35,7 @@ class ExampleComponent extends React.Component {
unsafe_componentWillReceiveProps(nextProps) {
// New name for componentWillReceiveProps()
// Indicates that this method can be unsafe for async rendering.
// Prefer static getDerivedStateFromNextProps() instead.
// Prefer static getDerivedStateFromProps() instead.
}
}
```
@@ -164,7 +164,7 @@ class ExampleComponent extends React.Component {
derivedData: computeDerivedState(this.props)
};

static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
static getDerivedStateFromProps(nextProps, prevProps, prevState) {
if (nextProps.someValue !== prevProps.someValue) {
return {
derivedData: computeDerivedState(nextProps)
@@ -359,7 +359,7 @@ class ExampleComponent extends React.Component {

## New static lifecycle methods

### `static getDerivedStateFromNextProps(nextProps: Props, prevProps: Props, prevState: Props): PartialState | null`
### `static getDerivedStateFromProps(nextProps: Props, prevProps: Props, prevState: Props): PartialState | null`

This method is invoked before a mounted component receives new props. Return an object to update state in response to prop changes. Return null to indicate no change to state.
Copy link
Member

Choose a reason for hiding this comment

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

Return null to indicate no change to state

What's the benefit of this API design?
What about undefined something like this?

static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
  if (someCondition) {
    return;
  }
}

// or
static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
}

I prefer to return prevState explicitly if we don't need to update the state.
Return null to indicate no change to state might be unclear for developers.

Also, waring for returning null or undefined is good for me.

static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
  if (!someCondition) {
  }
  return prevState;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Return null to indicate no change to state

What's the benefit of this API design?

It signals a clear intent. Undefined return value may indicate someone forget a default case or an else statement, etc

Copy link
Member

Choose a reason for hiding this comment

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

@bvaughn Thank you for your response!

What happened if getDerivedStateFromNextProps returns undefined? It is ignored and the state won't be changed?

It signals a clear intent.

In that case, Return prevState seems to be clearer than return null.
Why is null treated as a special case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem. 😄

What happened if getDerivedStateFromNextProps returns undefined? It is ignored and the state won't be changed?

Probably log a warning, suggesting the user return null instead, to avoid the potential accidental omission of a return statement like I mentioned before.

In that case, Return prevState seems to be clearer than return null.
Why is null treated as a special case?

I think null is safer. We could compare the identity of the returned value to prevState, but what if a user modified values on prevState (same object, but mutated keys - although to be clear, this should not be done).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Probably log a warning, suggesting the user return null instead, to avoid the potential accidental omission of a return statement like I mentioned before.

👍

I think null is safer. We could compare the identity of the returned value to prevState, but what if a user modified values on prevState (same object, but mutated keys - although to be clear, this should not be done).

I got it.
But why is comparing the identity of the returned value to prevState needed?
Will the result be used for something?
Are there any problems to use a value returned from getDerivedStateFromNextProps without the comparing?

const nextState = type.getDerivedStateFromNextProps(nextProps, prevProps, prevState);
if (/* validate nextState */) {
}
filter.memoizeState = nextState;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But why is comparing the identity of the returned value to prevState needed?
Will the result be used for something?

Because the returned value, like the value passed to setState, can be a partial state- meaning that we need to (eventually) merge it into the current state. Merging the current state with itself is doing work unnecessarily- and a subsequent call to render() (the default behavior when state is updated) is more work- so it's nice for us to have a faster way to determine there's no actual work to be done.

Copy link
Member

@koba04 koba04 Jan 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks That makes sense!I missed the return type is PartialState | null.
setState(updater) can accept undefined to indicate no change not only null so it might also make sense to accept undefined as well as a perspective of API consistency.

Copy link
Collaborator Author

@bvaughn bvaughn Jan 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That's not an unreasonable suggestion, but I disagree with it. To me, it seems more likely that one might forget to handle a return case than it is for one to accidentally call this.setState with an undefined value. React's DEV warnings are there to protect users from accidental oversights, and this (in my mind) is more similar to forgetting to return a value from render (which we also don't support).


@@ -383,7 +383,7 @@ This method will log a deprecation warning in development mode recommending that

### `componentWillReceiveProps` -> `unsafe_componentWillReceiveProps`

This method will log a deprecation warning in development mode recommending that users use the new static `getDerivedStateFromNextProps` method instead (when possible) or rename to `unsafe_componentWillReceiveProps`.
This method will log a deprecation warning in development mode recommending that users use the new static `getDerivedStateFromProps` method instead (when possible) or rename to `unsafe_componentWillReceiveProps`.

`componentWillReceiveProps` will be removed entirely in version 17.