-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
componentDidReceiveProps Please #3279
Comments
Why not |
Looking through my own projects I can see I've done this where I had similar needs (another one is deriving a chunk of state based on props), just passing in a different set of props when needed so you only have to pass something extra when reusing, without duplicating knowledge of which props are needed: setup(props) {
props = props || this.props
UserActions.load(props.id)
} |
@syranide The trouble is when setup needs to call methods that also need props, which needs to call methods which also needs props... Eventually your entire component is plumbing around props. |
+1 seems like a bunch of needless wiring up in the app with the current pattern. Could be a concise standardized way to solve the problem. Seen a bunch of people get burned on this.props inside of ComponentWillReceiveProps, a clear sign that it's not intuitive. |
No longer find this to be an issue. |
+1, I am also finding myself passing around props a lot. Similar to @insin above I was using default params for awhile:
But decided it was an anti-pattern due to the subtle bugs it can cause if you forget to pass newProps. |
+1 |
I think the reason it's not available is because Most of the times, if you're using |
+1 |
+1 I want to be able to respond to the new props event when |
+1 |
1 similar comment
+1 |
What about |
Use |
+1 |
+1 this helps with DRY code and simplifying logic. I would be open for componentDidReceiveProps to be called on the initial setup to make the example starting this thread just a single function:
|
Thoughts? componentWillReceiveProps() {
setTimeout(()=> {
if(this.isMounted()) {
this.componentDidReceiveProps();
}
});
}
componentDidReceiveProps() {
UserActions.load(this.props.id);
} What do you think? |
@YourDeveloperFriend I think it depends on how it works relative to other lifecycle hooks and rendering delays due to cascading timeouts on nested components. These sorts of lifecycle hooks should be called synchronously in a pass that is guaranteed to be called before render. I haven't studied React's codebase, but I'm assuming render isn't called on a timeout. Most likely, the best solution is going to be something along the lines of registering pre-rendering hooks or to mark components that have had their props changed so the render logic calls componentDidReceiveProps before calling render. |
Nah, I'm good. There are better solutions. |
+1 |
When you do things like
you introduce statefullness either in the component or (much worse) outside. Why do you ever need to
@robyoder , passing arguments to functions isn't ugly. It may seem too verbose but it's natural in the programming language you've chosen. Trashing the API by adding a spare lifecycle method just to reduce verbosity is definitely ugly indeed. |
So tell me, @me-andre, why do we have both In my opinion, it's because they serve different purposes and both are helpful. Just like those two are not the same, The benefit of the previous props is that you wouldn't have to load data every time; it can be conditional based on the props and whether they've changed. Obviously, "[ugliness] is in the eye of the beholder", because adding a proper lifecycle method seems like a much cleaner solution to me. |
Maybe there is another way to look at this... Let's say the main goals here is are: (1) reduce human error - I forgot to pass parameters or use So maybe the solution space is around simplifying of both the use of React and the React API itself to achieve these goals. If you think about the problem with these goals in mind, maybe some lifecycle methods could be merged and if you are interested in knowing it is initialization vs update, you can tell by the calling signature / arguments. For example: Personally, it feels like there are too many lifecycle methods and if they were more called consistently (without or with prevProps / prevState on initial pass and update), it could simplify my code and increase my productivity. Plus, the lifecycle method names are pretty long (I tend t copy / paste them around rather than typing them) and it is hard to remember which will / did's exist which makes me think they could / should be simplified. |
@robyoder , the short answer why we have both
As you may have noticed there are lifecycle points / conditions where one method gets called and the other does not. That's why we have 2 distinct lifecycle methods. However |
@me-andre you might be responding to me (@kmalakoff) and @robyoder at the same time. Or maybe not, but I'll take this opportunity to move the discussion forward... What I was raising was that perhaps thinking above the current API with these three above goals could give new insights or perspectives into how one might simplify the API to achieve them. Of course, after going through the exercise, we might end up at the same place. Let's go through the exercise.... Let's assume this topic is being followed and +1-ed because there is something important to it and let's say the addition of Keeping in mind the table and the 3 goals I put forward, does anyone have any ideas for simplify the API and / or another way to give the people in this thread what they desire and to not expand the API (maybe even reducing / simplifying it)? |
@kmalakoff , the 3 points you talk about are connected with the API only in that you use the API when you encounter them. They aren't caused by a bad design. The first problem you talk about is that lifecycle methods take different arguments. Well, that's perfectly natural - they serve different purposes. Regarding "there are too many lifecycle methods" - not yet and won't ever be if you stop requesting for new callbacks for which you don't have a technical need. Technical need is when you implement a complex component and in the middle of work you understand there is absolutely no way of doing it without some ugly hack. It's about "to trigger a tricky DOM method I need something to get called at the time the component does X but there's no such method and I cannot add one, so I use setTimeout and hopefully it wouldn't land earlier than I need". Not when you say "oh, I got tired of writing props = this.props". And one more thing... |
@me-andre based on my experience working with React, I believe there is room to improve / simplify the API. I'm not alone which is why this issue was raised in the first place and is being +1-ed. If I answer by providing feedback on your analysis, I don't think it will really move this forward (which is why I have avoided doing it so far) since it is a bit biased towards the status quo and justifying the current API, but I'd be happy to provide feedback on potential solutions! People engaged in this issue are looking to explore improvement ideas and potential API changes to address the issues raised through their experience using React (like I outlined above). Maybe you are right that the community should do the innovation to move the API forward and maybe have it rolled back into core or maybe the current API is perfect as is, but I think this is a great opportunity to consider what change may look like with the React team. Maybe the reason you are making arguments as you are is because we are just users of the library with less depth of understanding on the currently API and decisions that went into it. Maybe you could pull in some other React team members who is equally experienced and knowledgeable as yourself to see if we get a different perspective and maybe even come up with a great solution or to get consensus that this is as good as it gets? |
@kmalakoff , imagine React is an engine and 4 wheels. Now every time you want to go somewhere, you build the rest of the car and that eventually starts being annoying. Otherwise you complain about the need to remember low-level engine details and turning front wheels by hands doesn't feel the right way. |
@me-andre I believe we understand your position and your line of argument well now. Thank you! You may be right that the API is already as good as it will get, but there is also the possibility that it can be improved. Can someone make a case for changing the API? eg. providing comparative analysis of various options (add componentDidReceiveProps vs merge / simplify APIs vs no change) |
The React team has been watching the issue, and we do discuss it internally. In principal, I always lean toward eliminating API surface area, so I find myself leaning toward @me-andre's arguments. However, our team sometimes affectionately refers to What would be most useful for us (or at least me) is to have a solid understanding of WHY people want this lifecycle method. What are you trying to do in this lifecycle that isn't sufficiently covered by the other lifecycle methods? Why would someone want to do |
@jimfb I've given some thought to this after you suggested I move this to the gist and actually do not believe this line of analysis is off topic...the reason I am interested in this issue around received props is that I believe having a received props lifecycle method will allow me to reduce boilerplate. Please hear me...reducing boilerplate is the problem that the lifecycle method in this issue could solve for me. I believe that it also could be a significant part of the reason why people are interested in this lifecycle method because the current API encourages boilerplate more than we would like. That said, I'd be happy to try to decouple the API improvements and for example, only focus on exploring the solution space around improving the API related to props in this issue. |
@kmalakoff The topic of the thread is defined by the title and first post. The title is Your points are valuable, and I encourage you to have the discussion, just please move it to a different thread, because we want to keep github threads focused on the topic at hand. Otherwise the discussions are too difficult to track/manage. Threads do exist for most of the topics you raised. |
@jimfb why are people asking for You have asked for the problems and use cases, and I've stated this as the problem for me and shown a use case above (responding to @shea256) which seem on-topic, within scope and, from my perspective, important for resolving this issue. I have agreed to hold off on expanding the scope to general API issues / improvements and will. I promise! 😉 FYI: as I stated previously, the reason I started on the thought experiment around this issue was because the arguments seemed a little too narrowly focussed (eg. we can already do everything we need with the current API) rather identifying the underlying motivations behind the issue. As you know, sometimes you need to step back, then focus in, then view from a different angle, then revisit assumptions, investigate the underlying reasons behind the request, etc to iterate towards the best solution to a problem. Hopefully, I have helped draw attention to this and my participation will help get this issue resolved to our satisfaction! (of course, I'm also hoping the React API will evolve to be more use-friendly and will continue that discussion elsewhere...thank you for the links) |
@kmalakoff , ok, to reduce boilerplate you just make reusable classes or components from which you subclass or compose another components. Nobody prevents you from defining a common method for all components in your app. React does provide excellent extendability: you can define components as objects, functions or classes. You can extend components by subclassing, mixins or factories. |
@me-andre that absolutely is an option if this issue does not lead to an API improvement. This issue was raised to promote discussion around a desire to make a change to the API to improve it. Exploring alternatives in client code definitively should be considered, but if a strong case is made that the API should be changed, these sort of workarounds would be unnecessary. In order to counter argue, cases also need to be made for what an improved API should be and then evaluated against the current API to assess them. For example, if you were to demonstrate that the current API without defining a custom superclass could be used in a way to reduce boilerplate (eg. we are using the API incorrectly, or there are features in the current API that can be used to achieve a similar level of reduction of boilerplate, etc) or prove why there is no improvement possible (eg. no general solutions are exist because there is a major use case that could not be supported in our API improvement options), etc, you could make a case that the API is good enough as is. If the API requires people to write custom superclasses for basic and common control flow patterns, it strengthens the argument that there is a case for the API to be improved. |
Curious - what is the rationale for not having What are the use-cases where you would only want to trigger something upon receiving new props (and not on the initial prop reception)? I could be missing something obvious here but just trying to reconcile the viewpoints of the various parties. In any case, if @kmalakoff if |
|
@shea256 good points. Having different code paths for initialization vs changes is one of the root causes of prop boilerplate. The other root cause is having with different signatures for handling props like @akinnee-gl points out. This is why I'm trying to expand the solution space being considered (eg. also call on init) since there could actually be an even better solution to reduce prop boilerplate more. I hope we will be able to get further: Before New Hook
After New Hook (Revised)
or if UserActions.load cannot check the currently loaded id:
|
@kmalakoff , what I'm talking about is that API improvement is absolutely available for you right now: you can make your own factory of components and then share with us (along with use case samples). This will make your proposal and rationale hundred times more clear. Since all lifecycle points already have appropriate callbacks, you can introduce any new method at any lifecycle point / component state. You can even mixin an event emitter into your component and make it possible to attach several handlers for a state change. @shea256 , one possible reason for @kmalakoff , I've twice posted code samples which show how to organize a React component in a way that it doesn't need any @akinnee-gl , the reason not to introduce a new method for just accessing
I just don't see how it simplifies things in the React ecosystem, but that is what you keep requesting. |
@me-andre the premise of this issue is not that we cannot achieve what we want with the current API nor that we cannot work around the current API in client code. The premise of this issue is that the current React API is sub-optimal and needs to be improved; for example, if you were to come up with principles of what an optimal API would look (like I tried to above), the current React API would score in a mid-level range because it is suboptimal / deficient in a number of areas. Unfortunately, providing ways in client code to work around the React API does not address the root causes of the problem, shifts the debate away from addressing the underlying the issues, and will not lead to a debate around potential solutions to improve the React API. In short, I already have workarounds that are working for me because I have a bunch of React apps in production so best practices could be great for a blog post, but using them as a line of debate in this issue will just sidetrack us from a real debate on the true purpose of this issue: how to improve the React API (in this issue, limited to prop use cases and boilerplate). React 1.0 should aim for the top, not the middle. A major version increase can break backwards compatibility so let's go for the best possible API based what we've learned from all of these years of using the 0.x version. (FYI: I think people might not be engaging with your examples as much as you are hoping because they are not coming here to get taught about the current API, but because they are looking for / hoping for improvements to the API, eg. they could be perceived as with good intentions, but being slightly misaligned) |
Ok, you come with an API improvement proposal, but then you show the code which is way too far from "React best practices". Some things seem very much like worst practices. Then you tell: "that is the reason for a change". Yes, that is the reason for a change but not in the React codebase.
When you wrap a low-level API with a higher level API that is not a workaround, but a common practice. Many brilliant frameworks follow that idea.
What you commonly do when you architect/design an API is predicting problems, hypothesizing about use-cases etc. The reason why people hypothesize is not that they try to abstract themselves from a real world in search of the ideal. It's just lack of experience - you can't get experience "in advance". You tell you have that experience, you've seen real problems and you have some workarounds you can share. That is exactly what can turn this debate into being "real" and "effective". The React itself was built on real problems and challenges - that's why it solves real architecture problems and not just "how to write hello world in 3 lines". |
@me-andre I hear you and your line of argument is clear. You are correct that central to my argument is that if we establish better principles based on our collective experiences of using the current React API and open up debate to include non-dogmatic solutions that may change the API in some fundamental ways, we can and should seize the opportunity to improve the React API to make it even better than it is today. Let's not rest on our laurels when there is room to improve! How would you rate the current React API around props? (let's say using letter grades: F to A+), why, and what would you do to improve it if it is less than an A+? |
@me-andre have you had a chance to prepare your ranking and analysis? I'm interested in hearing what you believe are the strengths, weaknesses, and opportunities with the current API. |
+1 |
+1 Please |
Is there a workaround? I need ComponentDidReceiveProps |
I made this issue over a year ago and have been using React daily since. I no longer think there is a use case for componentDidReceiveProps that justifies increasing the API. @AlexCppns what is it you are trying to do? |
@iammerrick, actually it's alright, I just misunderstood how componentWillReceiveProps is used. |
Related discussion: #7678 |
I've ran into this a few times, and what I ended up doing was: componentWillReceiveProps(nextProps) {
if (this.props.foo !== nextProps.foo) this.needsUpdate = true;
}
componentDidUpdate() {
if (this.needsUpdate) {
this.needsUpdate = false;
// update the dom
}
} It's not too bad. |
@brigand , there's no need for a flag - you can compare props within
Furthermore, you solution could be easily broken by |
Oh cool, thanks! |
@jimfb I think I have a synchronous use case below. I think a
|
@chanakasan , your example lacks
|
Hi @me-andre, Thanks for taking the time to reply. I've read the discussion on this page and I like to thank you for participating in it. Yes, a I did think and try the two things you've suggested before, but failed.
I just created a complete example from my use case. I've made it easy to understand much as possible. If you could please have a look and point me in the right direction. If I am able to understand this properly I'll share this example in a blog post to help others as well. |
@chanakasan, come on, this looks like a production code. I'm not gonna read all of it and help you with your project for free. However I can point you to the right direction:
|
@me-andre I think I understand your point about
But the caveat of using I'll try to follow your advice. Thanks I appreciate it. Btw, it wasn't my intention for you to help with my production project for free :). It's a more full example from my previous short example to fill in any blanks about my use case. |
what if we use this in conjunction with shouldComponentUpdate? my work around is to set this.props to the new props before running the desired function |
It really would be nice to have the ComponentDidMount or some hook for that. Why? Because, some times, we need to do other calculations which doesn't depend on React lifecycle. For example: I have a parent component which may be resized. The child component is responsible to render an OpenLayer map which has a function that is responsible to resize the map. However, it should happen after the child get props from the parent and also committed other calculations within React lifecycle. |
I would like to humbly request a componentDidReceiveProps hook, often times I would like to do something on both componentWillMount and componentWillReceiveProps, but because
this.props
hasn't been set yet I am forced to pass props around instead of reading directly fromthis.props
.Before New Hook
After New Hook
In this simple example it may seem like a small thing, but often times the passing of props runs deep and instead of conveniently referencing this.props one is forced to plumb the props throughout the component.
Please consider adding
componentDidReceiveProps
as a hook to leverage the same code that is leveraged in componentWillMount without forcing both to plumb props throughout the component.The text was updated successfully, but these errors were encountered: