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
prevProps
in getDerivedStateFromProps()
#40
Conversation
Wanting to proactively keep my code up-to-date with latest best practices, I have been refactoring I don't want to fuel old discussions which have been had elsewhere already, but the solution here of passing The other reason given for not passing |
} | ||
``` | ||
|
||
The whole `constructor()` thing is just for `prevProps` and feels bulky. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example seems a bit contrived, since you can write this all as:
state = {}
static getDerivedStateFromProps({ country }, state) {
if (country !== state.country) {
return { country, derivedValue ... }
}
return null
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we'll need some real-world examples to motivate this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jquense I can, but essentially this would be the same thing: storing part of previous props
inside state
- that's what you're basically suggesting. In my case I don't even need that country
in my state - it's just the fact that it changed, nothing more. Consider the externally set default country, which can or can not change the currently selected internal state country depending on circumstances. Or consider localizedMessages
: i don't need to store them in state, I just re-generate <select/>
options based on those messages if they changed, and that's it.
const new_default_country = this.props.country
const old_default_country = prevProps.country
const country = this.state.country
// If the default country changed.
// (e.g. in case of ajax GeoIP detection after page loaded)
// then select it but only if no phone number has been entered so far.
// Because if the user has already started inputting a phone number
// then he's okay with no country being selected at all ("International")
// and doesn't want to be disturbed, doesn't want his input to be screwed, etc.
if (new_default_country !== old_default_country && !country && !value) {
return {
...,
country : new_default_country
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaearon CC ^^^^^
Here's a real-world example I encountered a couple weeks ago while reviewing a PR to upgrade As you noted, it's not particularly burdensome to do as @jquense suggested and store the previous prop values within the component Here's an approximate recreation of the buggy static getDerivedStateFromProps(nextProps, prevState) {
const { client, query } = prevState;
// If we're being bypassed, return early.
if (nextProps.skip) {
return;
}
// Reset all component state if the Apollo client changes.
if (client !== nextProps.client) {
return {
...initialState,
client: nextProps.client,
};
}
// Dump our prior results if the query changes.
if (query !== nextProps.query) {
return {
lastResult: null,
query: nextProps.query,
}
}
} In case it's not clear, the problem with the above code is that There are two straightforward ways to fix this function, but both have drawbacks (in my opinion). The first approach is to adopt a builder-pattern for static getDerivedStateFromProps(nextProps, prevState) {
const { client, query } = prevState;
const nextState = {
client: nextProps.client,
query: nextProps.query,
};
// If we're being bypassed, return early.
if (nextProps.skip) {
return nextState;
}
// Reset all component state if the Apollo client changes.
if (client !== nextProps.client) {
return {
...nextState,
...initialState,
};
}
// Dump our prior results if the query changes.
if (query !== nextProps.query) {
return {
...nextState,
lastResult: null,
}
}
} I could see a world in which linters enforce that The alternative approach I've seen in the wild (and was indeed suggested by @catamphetamine in this RFC) is to just include the entirety of static getDerivedStateFromProps(nextProps, prevState) {
const { prevProps } = prevState;
const nextState = { prevProps: nextProps };
// If we're being bypassed, return early.
if (nextProps.skip) {
return nextState;
}
// Reset all component state if the Apollo client changes.
if (prevProps.client !== nextProps.client) {
return {
...nextState,
...initialState,
};
}
// Dump our prior results if the query changes.
if (prevProps.query !== nextProps.query) {
return {
...nextState,
lastResult: null,
}
}
} This approach is a bit simpler, but still requires the The (admittedly minor) problem I see with this approach concerns PureComponents. Depending on React internals, the I think the coding conventions I proposed above are adequate and workable; however, I don't think this API sets developers up to fall into the 'pit of success', so to speak. In fact, even after fixing and simplifying the codepaths through |
|
||
// Needs to store initial `this.props` here | ||
// so that it's not `undefined` on the first | ||
// `getDerivedStateFromProps()` call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example code wouldn't actually work. If "prev props" are the same as props
during the initial render- the props.country !== state.props.country
check below would never be true and the initial render would never have access to the derived state
values until the first time the component was updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bvaughn Actually, yeah, I should have wrote this.state = { props, derivedValue: ... }
there instead of just this.state = { props }
. I think if I push the correction now it will destroy comments so I guess I won't. If the constructor code is corrected to this.state = { props, derivedValue: ... }
, and then if prevProps
are implemented as the third argument of getDerivedStateFromProps()
, then it will still make sense because developers will be able to drop props
from this.state
resulting in less cluttered state and avoiding the cases when a developer accidentally forgets to return props
as part of new state
in some of the many if/else
branches of getDerivedStateFromProps()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with catamphetamine. The derivedValue
could just have an initial value in the constructor. If the derivedValue
issue is solved, could prevProps
be provided using the value of props then?
|
||
// `state.props` is `prevProps`. | ||
static getDerivedStateFromProps(props, state) { | ||
if (props.country !== state.props.country) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words- if props
and prevProps
are the same for the initial render, this check won't work. That's why prevProps
would need to be null or undefined the first time (if that parameter were to be provided).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words- if props and prevProps are the same for the initial render, this check won't work.
If props and prevProps are the same for the initial render, this check won't "won't work". It will work. It just won't enter the if
condition. Ain't nothing illegal in that.
That's why prevProps would need to be null or undefined the first time (if that parameter were to be provided).
Since your previous statement is falsy, this statement you're conducting from the previous one is also false.
prevProps
would not need to be null or undefined the first time.
prevProps
will simply be equal to this.props
the first time.
And that's it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code, as written when I left this comment, would not work- because it would not set derivedValue
anywhere for the initial render. That is all I was pointing out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bvaughn ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bvaughn I'm not mad at you, by the way. Just to make things clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about setting prevProps
to {}
on the first render?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@j-f1 I think it would break someone's code, because they could have
static propTypes = {
value: PropTypes.shape({
nestedValue: PropTypes.object
}).isRequired
}
So they could expect props.value
to always exist, and a developer could query props.value.nestedValue
without any if/else
s, so empty props would throw Cannot read "nestedValue" of undefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part looks reasonable:
If props and prevProps are the same for the initial render, this check won't "won't work". It will work. It just won't enter the if condition. Ain't nothing illegal in that.
But I'm not sure what's the conclusion of this statement? If this statement is valid, could it be the reason to agree suing props as prevProps's first value?
Thanks for the detailed comment, @jamesreggio! ❤️ I think you raise a good point about there being potential for a subtle "memoization" type bug with this pattern. I want to point out that
In the above example, static getDerivedStateFromProps(nextProps, prevState) {
const nextState = {
client: nextProps.client,
query: nextProps.query,
lastResult:
nextProps.query !== prevState.query ? null : prevState.lastResult
};
// Reset all component state if the Apollo client changes.
if (nextProps.client !== prevState.client) {
Object.assign(nextState, initialState);
}
return nextState;
} 1: If it isn't expensive to derive "state" for a certain prop, then you can just do it in render and skip the Edit: My initial suggested re-write didn't account for state reset when |
This is my subjective opinion and you can ignore if you want. I think the problem with |
There is a condition you didn't mention:
|
This is only necessary if one of the two conditions I mentioned above is true.
I don't see how what you mention above is a new condition. (I think the two conditions I mentioned are sufficient, but please correct me if I'm misunderstanding.) |
@bvaughn |
I don’t understand this comment. You’re not supposed to add We’re not encouraging you to add |
What you're describing sounds like it would fall under the first category I mentioned above:
The phone number you're storing in state isn't really relevant to mirroring a prop, because you would (presumably) have to store it in state either way- unless I'm misunderstanding your example. |
@gaearon my take on main concern of not having Lets say I can derive B from A, but it's not trivial and I don't want to recalculate it unless A changed. But in order to know if A changed, I need to store it in state, and that means that (And that might also means |
I think it's important to point out two things:
So that's still a worthwhile goal. 😄 |
I'd like to add another example where this RFC would help. I have a custom Before (using constructor(props) {
super(props);
this.state = {
textWidth: computeTextInputWidth(props.min, props.max, props.step),
textValue: '' + props.value,
};
}
componentWillReceiveProps(nextProps) {
const nextState = {
textValue: '' + nextProps.value,
};
if (
nextProps.min !== this.props.min ||
nextProps.max !== this.props.max ||
nextProps.step !== this.props.step
) {
nextState.textWidth = computeTextInputWidth(nextProps.min, nextProps.max, nextProps.step);
}
this.setState(nextState);
} After (using constructor(props) {
super(props);
this.state = {
min: props.min,
max: props.max,
step: props.step,
textWidth: computeTextInputWidth(props.min, props.max, props.step),
textValue: '' + props.value,
};
}
static getDerivedStateFromProps(nextProps, prevState) {
const state = {
min: nextProps.min,
max: nextProps.max,
step: nextProps.step,
textValue: '' + nextProps.value,
};
if (
state.min !== prevState.min ||
state.max !== prevState.max ||
state.step !== prevState.step
) {
state.textWidth = computeTextInputWidth(state.min, state.max, state.step);
}
return state;
} If this RFC were implemented constructor(props) {
super(props);
this.state = {
textWidth: computeTextInputWidth(props.min, props.max, props.step),
textValue: '' + props.value,
};
}
static getDerivedStateFromProps(nextProps, prevState, prevProps) {
const state = {
textValue: '' + nextProps.value,
};
if (
nextProps.min !== prevProps.min ||
nextProps.max !== prevProps.max ||
nextProps.step !== prevProps.step
) {
state.textWidth = computeTextInputWidth(nextProps.min, nextProps.max, nextProps.step);
}
return state;
} Having access to |
I have not said we should implement the As @gaearon say, we will only need
Unless you are going to tell us the next version,
then I believe we(at least me) should not have the problem when looking at the example/suggestion of In short, it's mainly about the symmetry and the habit of traditional/classical thinking of |
@nwoltman You don't need to duplicate logic in state initialization. This can be achieved with the first gDSFP call. Use something like default values. this.state = {
min: 0,
max: 0,
step: 0,
textWidth: 0,
textValue: '',
}; |
I think the central theme of the most recent example is convenience? The difference between what's current and what's proposed is fairly trivial. Personally, I don't think this is a convincing enough reason to reverse the decision that was made on the RFC. (The convenience aspect has been discussed on the RFC and on some GitHub issues (e.g. facebook/react/issues/12188, /issues/27).) |
@bvaughn |
@TrySound True, I could do that when initializing state, but my main issue isn't really code duplication, it's more the fact that the properties are duplicated in both Also, there is actually a bug with that solution. If the component receives Initializing state with essentially "dummy values" feels like an anti-pattern to me. It seems useless to set values that are just going to be replaced when I think the developer experience around
2 becomes trivial once 1 is implemented since @bvaughn In addition to convenience, it also saves on the amount of code that needs to be written (so less bytes need to be sent to the client). |
RFCs exist so the community can share feedback about new APIs. I wrote this particular RFC, so I do understand it, and I've read and responded to every piece of feedback on it (including ancillary issues like this), so it's quite reasonable for me to link to previous, similar discussions rather than repeat answers already provided. You don't have to read the previous discussion of course. That's your choice, although you may miss out on some context if you don't. But please, let's keep this discussion friendly.
As for the concern that saving memory is a bad argument because React passes
True, although this difference is also trivial in the context of an entire application. |
@bvaughn So it's just keeping the API minimal then. getDerivedStateFromProps(props) {
return {
derivedValue: derive(props.value)
}
} Then there's really no need for render() {
const derivedValue = derive(this.props.value)
...
} So there's no real usefullness in using Sidenote: Well, maybe there is usefullness in a sense that Furthermore, dropping Now, my point is that when deciding on the Perhaps |
Calm down first. I think we should have a poll in somewhere and voters have to provide an example. I would recommend React Core team should do it. Otherwise we do not have enough voices/examples to drive this. |
Unless i'm missing more context the argument against |
Actually, yeah, I'd vote for a poll. |
@bvaughn Aha, I see, thx. If React had some version number detection (which could perhaps be added to I have updated the first post in this thread with the new text clarifying the backwards compatibility issues.
If you have anything to add to / change in the first post in this thread then leave a comment. |
I am not sure, but what it example with memoization inside getDerivedStateFromProps, not render, could change some mind? import memoize from "lodash.memoize";
// "one for all" memoization
const memoizedFilter memoize(allItems => allItems.filter((thing) => !thing.hide));
class ListSomeThingsV1 extends Component {
state = {
// "per instance" memoization
filter: memoize(allItems => allItems.filter((thing) => !thing.hide));
}
static getDerivedStateFromProps(props, state) {
return {
filteredItems: this.state.filter(props.allItems)
}
// ps: you can compare oldState.filteredItems and newState.filteredItems to "detect" a change.
}
// ...
} |
@theKashey You could update the Maybe you would not even need to use a class component and could just use a functional one- which would be faster anyway, e.g.: import memoize from "lodash.memoize";
const memoizedFilter = memoize(allItems =>
allItems.filter(thing => !thing.hide)
);
const ListSomeThingsV1 = ({ list }) => {
const filteredItems = memoizedFilter(list);
// ...
}; PS. I probably wouldn't suggest putting a filter function in |
It is better to leave an important note about react and _memoization.
@bvaughn - what is actually the "proper" place to store memoized function - "this" or "this.state"? I personally prefer "state", as long you can access function from dev tools and inspect it. Or provide some useless information from another point of view. |
Yeah, Dan filed a follow-up issue for calling out the caching behavior 😄 Seems like I personally wouldn't recommend putting functions (memoized or otherwise) on |
For when I want WeakMap based memoization or even endless memoization I've been using memoize-immutable recently. I have a distaste for the lodash style memoize that uses serialization. There used to be a library that gave reselect a WeakMap based selector implementation, but the GitHub repo appears to have disappeared and the npm package corrupted. Though there may be a different library worth looking into: https://heygrady.github.io/redux-selectors/ |
If you want something very simple and memoizing based on the last value is all you need, https://github.com/alexreardon/memoize-one looks like a good candidate (per instance). |
All of the above memoizers also work well with core-decorators' import {decorate} from 'core-decorators';
import {partialRight} from 'lodash';
import memoizeOne from 'memoize-one';
import memoize from 'memoize-immutable';
class MyComponent extends Component {
// Memoized with a single cache per-instance
@decorate(memoizeOne)
computeSomethingForInstance(foo, bar) {
// expensive computation
return result;
}
// Memoized globally with a WeakMap for a single argument
@decorate(partialRight(memoize, {cache: new WeakMap()}))
static computeSomethingNonPrimitive(foo) {
// expensive computation
return result;
}
// Memoized globally with an unlimited cache for a single argument
@decorate(partialRight(memoize, {cache: new Map()}))
static computeSomethingPermanently(foo) {
// expensive computation
return result;
}
// See https://github.com/memoize-immutable/memoize-immutable#choosing-a-cache-store
// for other cache options involving multiple arguments, LRU maps, etc
} |
Whats to stop somebody from doing the following as a workaround to access previous props? Just tested as a thought experiment and it seemed to work for me. class MyComponent extends Component {
static getDerivedStateFromProps(nextProps, prevState) {
const prevProps = prevState.getOldProps();
console.log(prevProps, nextProps);
return null;
}
constructor(props) {
super(props);
this.state = {
getOldProps: () => this.props
};
}
render() {
return null;
}
} |
@sullivan-sean It isn't stopping anyone from anything. |
@sullivan-sean I don't believe React guarantees that |
@sullivan-sean That's a reasonable question 😄 The new lifecycle was made As Sebastian recently said, there are rules to React. At times they may feel limiting, but following them allows React to do many complex things for you (even more so with upcoming async features). |
A warning though: the new React 16.4 might break your |
That is a mischaracterization of the changes made in 16.4, as the thread you linked to shows. Let's keep the conversation about that issue on that issue and not expand it into unrelated threads (like this one). |
@bvaughn can you explain issues you have told a little bit more. For example why shouldComponentUpdate had not been moved into static method too (having that it has aceess to this and could be used in improper way?) |
Still cant get idea why eslint rule anecdotelly better |
Our decision was a pragmatic one. We looked at a lot of React components (within Facebook and on GitHub) and we saw that We didn't see this kind of misuse with We have a pretty high bar for changes like this because they cause a lot of churn (within the open source community and within Facebook).
Don't really understand what this means. |
got you, Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, i guess that is a little better, lolol....😉, But 4real though nice pull on the decision of changes.
I just found the thread today, completely read it, and have several questions:
If someone is writing The question is: am I correct about the above point?
could be, and might possibly be:
Because, The question is: is my example code making sense?
According to this, that's definitely why many people (including me) are eager for The quesiton is: is the above making sense? And the final question: if the above are correct and making sense, could As for my own use case, I'm building a chat app, and I'd like to know the difference of previous and current chat log length before rendering. What I'm trying to do in
So that I can render the log list with latest logs, and scroll to my desired log index. However, at the moment, I can only do this in |
Hey @marsonmao, What you're saying make sense. I understand that explicitly tracking e.g. I don't think the points you've raised are new though. (Similar issues have already been discussed above.) And I don't see much value in repeating covered topics. It just makes a long thread longer. 😄 In most cases, there are alternatives to |
@bvaughn Thanks for the reply! Yeah I think I basically agree with you, the post is long enough, but I think my use case is new, and it should be good to provide more real examples like that. And I've read the post, it's very helpful, truly clarify several misconceptions, but I think it did not cover types like my use case, which requires to know the difference of props changes. And, the 4 proposals are discussed, but only the " My proposal is not new, that's true, and I think I asked wrong questions. Oh and, before asking the new question, I should state that I totally agree with NOT ADDING P.S. I've seen the comment but it's the same topic so I only post here 😁 |
Thanks for being understanding.
Honestly, I don't believe this change will ever be made. It would cause too much confusion and churn in the ecosystem (as explained in this comment). My hope going forward is that fewer components use derived state, and so the awkwardness of using it becomes less important.
But if, hypothetically, we did add this parameter in the future– the downside of the approach you mention would be that it relies on you to initialize the entire Even your example above looks problematic to me: state = {
derivedValue: -1, // or some 'default-string' or anything
}
static getDerivedStateFromProps(nextProps, state, prevProps) {
if (nextProps.value !== prevProps.value) {
return {
derivedValue: fn(prevProps.value, nextProps.value)
}
}
return null
} Presumably "-1" is not a valid But I don't think there's a lot of value in discussing hypotheticals here. 😄 Have a nice day! |
That's totally fine.
I think this is okay, assign member variables (state object is one) initial values in the constructor is very intuitive I guess? Maybe the problem is that
I can live with that, if it's the library authors' suggestion. But I think the people calling for P.S. I'm not discussing how to DO it, the way to do it is like dicussion bofore, like copying props into state or writing wrapper function or whatever.
Maybe or maybe not I think, the value of I'll give my real example here to leave some record. I'm building a chat log list, it behaves like Slack, when you scroll upwards and is at scroll top 0, it fetches more logs, after fetch complete the scroll top should be adjusted to a proper location so that the view stays at the log that trigger the fetch (Anyway it's like Slack, if I'm not describing it good enough). So I assign the scroll index to the difference of previous chat log length and the current length. And actually one of my attemp is to use react-virtualized.
I know it always render twice for each fetch, but somehow I need that to happen for item sizes to be correctly calculated, so it's...fine (not so fine though). But if I could do the same logic before render then it's not bad, at least the scroll index could be calculated in a better place. |
An update from April 21, 2018:
This RFC was aimed to start a discussion on the new
static getDerivedStateFromProps()
method replacing the oldcomponentWillReceiveProps()
method.A lot of people are using
componentWillReceiveProps()
to derivethis.state
values and to do various other things based on whether did the props change or they didn't.The new
static getDerivedStateFromProps()
method doesn't provide any access toprevProps
so currently we all have to hack around it by storing those "previous props" inthis.state
:This does work, but it introduces some unneccessary code which wouldn't need to be if there was a third
prevProps
argument in thegetDerivedStateFromProps()
function.The new lifecycle API feels inconsistent because of this: why does
componentDidUpdate(prevProps, prevState)
haveprevProps
argument but at the same timegetDerivedStateFromProps()
refuses to provide theprevProps
argument?If
prevProps
argument is removed fromgetDerivedStateFromProps()
then it should also be removed fromcomponentDidUpdate()
but it's not and that feels weird.If the
prevProps
argument was provided bygetDerivedStateFromProps()
function then the example at the top would look like this:Which is undoubtedly cleaner.
Still, as the discussion proceeded, the library authors pointed out that adding a third
prevProps
argument in the next React version (say,16.4
) would not be of much use because thereact-lifecycles-compat
polyfill only polyfills for React <=16.2
while for React16.3
getDerivedStateFromProps
is left untouched and that's why it would break on React16.3
if it relied on the newprevProps
argument being present. So, even if theprevProps
argument was added developers wouldn't be able to use it in their libraries unless they'd like to break compatibility with React16.3
and release a new major version of their library with apeerDependency
of React>=16.4
and maintain two versions of the same library: one for React<16.4
and the other forReact >= 16.4
(which is not a viable solution and which is exactly the opposite of what the polyfill originally aims to provide — backwards compatibility with all versions of React). It's a bit tricky, but seems that it's a valid point. See the library authors' comment. If React had some version number detection (which could perhaps be added to16.4
but that would be too much hassle I guess) then the polyfill could differentiate between16.3.0
/16.3.1
and all further versions of React (>=16.4
) so that it would polyfill on React<=16.3.1
and not polyfill on React>=16.4
.In the end, the library authors decided to close this RFC. I have no hard feelings about that, given that I've already rewritten my libraries with the
this.state.props
workaround.See the original RFC
(the original RFC has a couple of errors/typos but I chose not to push the corrections because that would destroy the comments by the reviewers)