facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.58k stars 46.79k forks source link

componentDidReceiveProps Please #3279

Closed iammerrick closed 8 years ago

iammerrick commented 9 years ago

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 from this.props.

Before New Hook

componentWillMount() {
  this.setup(this.props.id);
}

componentWillReceiveProps(next) {
  this.setup(next.id);
}

setup(id) {
  UserActions.load(id);
}

After New Hook

componentWillMount() {
  this.setup();
}

componentDidReceiveProps() {
  this.setup();
}

setup() {
  UserActions.load(this.props.id);
}

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.

syranide commented 9 years ago

Why not setup(props)?

insin commented 9 years ago

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)
}
iammerrick commented 9 years ago

@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.

JakeLingwall commented 9 years ago

+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.

gaearon commented 9 years ago

+1, I also find this frustrating.

No longer find this to be an issue.

dandelany commented 9 years ago

+1, I am also finding myself passing around props a lot. Similar to @insin above I was using default params for awhile:

setup(props = this.props) {
  doSomething(props);
}

But decided it was an anti-pattern due to the subtle bugs it can cause if you forget to pass newProps.

monsanto commented 9 years ago

+1

gaearon commented 9 years ago

I think the reason it's not available is because this.props and this.state always correspond to rendered values. There's no way to implement componentDidReceiveProps that doesn't trigger another render without breaking this constraint.

Most of the times, if you're using componentWillReceiveProps, you actually want either a higher order component a la Relay, or something like observe hook proposed for React 0.14.

mienaikoe commented 9 years ago

+1 Also, you can implement componentDidReceiveProps without changing this.props or this.state. If all you need to do is read from these, then you won't trigger another render. If you're writing to props or state within this proposed function call, then you're shooting yourself in the foot, but that's the same case for every other method in the lifecycle.

tristang commented 9 years ago

+1

I want to be able to respond to the new props event when shouldComponentUpdate returns false, so in my case I can't use componentDidUpdate.

dev-rkoshlyak commented 9 years ago

+1

jnmandal commented 9 years ago

+1

quantizor commented 9 years ago

What about componentWillMount and componentWillUpdate @iammerrick

gaearon commented 9 years ago

I want to be able to respond to the new props event when shouldComponentUpdate returns false, so in my case I can't use componentDidUpdate.

Use componentWillReceiveProps?

devjoca commented 9 years ago

+1

kmalakoff commented 9 years ago

+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:

componentDidReceiveProps() {
  UserActions.load(this.props.id);
}
YourDeveloperFriend commented 9 years ago

Thoughts?

componentWillReceiveProps() {
  setTimeout(()=> {
    if(this.isMounted()) {
      this.componentDidReceiveProps();
    }
  });
}
componentDidReceiveProps() {
  UserActions.load(this.props.id);
}

What do you think?

kmalakoff commented 9 years ago

@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.

robyoder commented 8 years ago

+1 please. Having the same issue where I have to pass props around to just about every method in my component class. So ugly.

Nah, I'm good. There are better solutions.

micky2be commented 8 years ago

+1

me-andre commented 8 years ago

componentDidReceiveProps() already exists: it's called render(). However there can be a case (like in @iammerrick 's example) where you need to load/trigger/etc something before you perform a render. This means one and the only one thing: you do something wrong.

When you do things like

setup(id) {
    UserActions.load(id);
}

you introduce statefullness either in the component or (much worse) outside. Why do you ever need to load() data every time a component receives props (they aren't even guaranteed to be new)? If you do lazy-loading, the proper way is to request the data in the render() method:

render() {
    var actions = UserActions.load(id);
    if (actions) // render
    else // show spinner or return null, etc.
}

UserActions.load = function(id) {
    if (data) return data;
    else // request the data, emit change on success
}

@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.

robyoder commented 8 years ago

So tell me, @me-andre, why do we have both componentWillUpdate and componentWillReceiveProps?

In my opinion, it's because they serve different purposes and both are helpful. Just like those two are not the same, render is not the same as the hypothetical componentDidReceiveProps because it's called for reasons other than new props; plus, you aren't given access to the previous props.

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.

kmalakoff commented 8 years ago

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 props = props || this.props yet again (2) to reduce non-value adding boilerplate code - why write something unnecessarily (3) to reduce the cognitive burden of trying to decide which lifecycle methods to use - why do I need to think about this sort of thing, why do I keep using slightly different approaches depending on how I feel that day, etc

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: beforeRender(prevProps, prevState) or update(prevProps, prevState).

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.

me-andre commented 8 years ago

@robyoder , the short answer why we have both componentWillUpdate and componentDidReceiveProps is there is a huge difference between them. They're similar of course, but mainly in being prefixed with componentWill.

state has changed component did not update
componentWillUpdate() yes no
componentWillReceiveProps() no yes

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 componentDidReceiveProps() as it was described in this topic does not represent any component state or condition and it doesn't give an access to anything componentWillReceiveProps() does not. It just adds a slight syntactical sugar which may or may not seem useful or easier - this is a personal preference, not a technical requirement. And that is exactly the reason why it shouldn't be added: it's subjective. You say passing arguments is ugly - but you also said "[ugliness] is in the eye of the beholder". For me it seems like you've answered yourself.

kmalakoff commented 8 years ago

@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 componentDidReceiveProps is not a good idea because it increases the API surface area.

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)?

me-andre commented 8 years ago

@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. componentWillReceiveProps takes props not because the moon was full when this method was added - but because it's about receiving props which have not been yet assigned to the component. props are being passed only in methods where they (may) differ from this.props. This is actually a hint, not an issue. The problem # 3 is that it's hard for you to decide which callback / approach to use. Well, that's not really a problem until the methods I talk above have the same signature. Once you have both state and this.state, props and this.props which are equal (read meaningless) in most methods, then you get entangled in spare options and alternatives. The problem # 2 is about boilerplate code... well, React is a thin library - and it's beautiful, easy and strict because of that. If you want to make a wrapper which would simplify our lives and reduce the amount of code we write every day - why not do it? Publish it as your own npm that depends on react and you're done.

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... In a well-designed React application, you don't need most of the methods we talk about or you use them very rarely. getInitialState, componentWillMount, componentWillUnmount suffice 99% of the time. In my current project which is a relatively large commercial app, componentWillReceiveProps is used twice throughout all the app. I would not use it at all, but the environment (read browser) is imperfect - manipulating certain "stateful-in-itself" things such as scrollTop or relying on layout computations for future renders requires manual synchronizing between props transitions and DOM changes. However in most "normal" cases, you just don't need to know when props transition happens.

kmalakoff commented 8 years ago

@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?

me-andre commented 8 years ago

@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. What I would recommend is either to get a complete vehicle (a full-fledged framework) or to build the needed components in the way that you can reuse them over time. What I see in this thread is engine users complaining that the engine has no hydraulic system and no interior lights. What I feel is that we'd get a complete crap if we add these features to where they don't belong. React is not a framework: it's more a diff-powered rendering engine that exposes a powerful component system. It has low level and minimalistic API yet powerful enough to enable you to build literally anything on top of it. Please don't hesitate contacting me via email if you feel like I need to clarify something - I don't want this thread to become a tutorial.

kmalakoff commented 8 years ago

@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)

jimfb commented 8 years ago

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 componentDidReceiveProps as "the missing lifecycle method", and there are serious discussions about potentially adding it. The important thing is that we enable best practices without catalyzing bad practices.

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 UserActions.load(id); within componentDidReceiveProps instead of within render as suggested in https://github.com/facebook/react/issues/3279#issuecomment-163875454 (I can think of a reason, but I'm curious what your reasons are)? Are there any use cases other than initiating a data-load based on props?

shea256 commented 8 years ago

@jimfb I believe that componentDidReceiveProps is exactly what I need and that other methods are inappropriate, but I could be wrong. I'll happily explain my use-case.

I have a route in my react-router that looks like this:

    <Route path="/profile/:username" component={ProfilePage} />

I need to fetch the data for the profile from an external source, and in order to do this properly, I need to make an HTTP request in the componentDidMount method.

Unfortunately, when I navigate from one profile to another, React Router doesn't call the constructor method or the componentDidMount method again (of course this could be a bug but I'm going to assume it's not for now).

I thought of fixing this by using the theoretical componentDidReceiveProps. Unfortunately it does not yet exist, and componentWillReceiveProps will not serve my needs.

Any pointers on this would be greatly appreciated.

My hunch is that componentDidReceiveProps is exactly what I need.

jimfb commented 8 years ago

@shea256 Can you elaborate on why componentWillReceiveProps does not serve your needs?

jimfb commented 8 years ago

@shea256 Also, why does your component need to make an HTTP request? What does that http request contain? If data, why aren't you updating your component asynchronously when the data callback returns?

grassick commented 8 years ago

@jimfb The common case for me is when we have to load something asynchronously in response to a prop change, which involves setting some state to indicate that loading is happening and then when the data is loaded, setting that state.

Both mounting and receiving new props should trigger this same loading cycle, so componentDidMount and componentWillReceiveProps is the place to call the update function. render doesn't have info on whether it's a new prop, and anyway, setState from render is a no-no.

So, I have one function that does the loading. But I have to pass in props as a parameter and carefully avoid using this.props which are out of date for componentWillReceiveProps. This inevitably ends up causing bugs as you have to remember to use the passed in props or you get subtle one-behind errors when changes arrive. And the signatures of all the methods involved are more complex as the props need to be passed in.

Yes, it can be done with the current api. But it causes clumsier, error-prone code which is what React is so good at avoiding in general.

shea256 commented 8 years ago

@jimfb I need to get the new username in the routeParam and with componentWillReceiveProps I don't yet have it.

I need to make an HTTP request in order to load the profile data from an external source (the data isn't stored locally).

And I can update my component in the render method but it feels a bit dirty:

constructor(props) {
  super(props)

  this.state = {
    id: this.props.routeParams.id,
    profile: {}
  }
}

componentDidMount() {
  this.getProfile(this.state.id)
}

render() {
  if (this.props.routeParams.id !== this.state.id) {
    this.getProfile(this.props.routeParams.id)
  }

  return (
    <div>
      <div className="name">
       {this.state.profile.name}
      </div>
    </div>
  )
}
shea256 commented 8 years ago

@grassick wrote:

The common case for me is when we have to load something asynchronously in response to a prop change, which involves setting some state to indicate that loading is happening and then when the data is loaded, setting that state.

Yes, this is exactly my use-case.

Both mounting and receiving new props should trigger this same loading cycle, so componentDidMount and componentWillReceiveProps is the place to call the update function.

Well said.

calmdev commented 8 years ago

@shea256 can't this be done using componentWillReceiveProps instead?

constructor(props) {
  super(props)

  this.state = {
    id: this.props.routeParams.id,
    profile: {}
  }
}

componentDidMount() {
  this.getProfile(this.state.id)
}

componentWillReceiveProps(nextProps) {
  let { id } = nextProps.params
  if(id !== this.state.id) {
    this.getProfile(id, (profile) => {
      this.setState({ id: id, profile: profile })
    })
  }
}

render() {
  return (
    <div>
      <div className="name">
       {this.state.profile.name}
      </div>
    </div>
  )
}
jimfb commented 8 years ago

@grassick wrote:

The common case for me is when we have to load something asynchronously in response to a prop change, which involves setting some state to indicate that loading is happening and then when the data is loaded, setting that state.

@grassick wrote:

render doesn't have info on whether it's a new prop, and anyway, setState from render is a no-no.

Just spitballing here: If componentDidUpdate got invoked on initial render (in addition to subsequent renders), would that solve your use case? You could check if props changed and do all your data loading in componentDidUpdate, right? And in render, you would know you were loading data if this.state.profileName != this.props.profileName. Would that alternative be sufficient for your use cases?


Are there any use cases that people have which do NOT involve components loading data based on props?

shea256 commented 8 years ago

@calmdev hm, I believe you're right. I'll try that out.

@jimfb perhaps, although I tried using componentDidUpdate and I thought it didn't work. I can take another look.

And it sounds like I totally could do a check on this.state.profileName != this.props.profileName but this also seems like a hack, doesn't it? At this point, if componentWillReceiveProps(nextProps) ends up working, I'd just prefer that. Now, what bothers me is the lack of symmetry with componentDidMount. Could I use componentWillMount in lieu of componentDidMount?

I just feel like the whole lifecycle could be cleaner.

me-andre commented 8 years ago

@shea256 , I have a question for you... have you read the README for React? I don't feel comfortable saying that but if not, you possibly shouldn't request new features for the tool you're not familiar with. For me that even sounds... absurd. "Could I use componentWillMount in lieu of componentDidMount?" No you can't because as it's stated in the readme, componentWillMount is called before your component gets to the DOM and componentDidMount - after that. Well, you definitely can replace one method with another and that would just break your code. The reason we have 2 methods here is not aesthetics (read "symmetry"). It's because we may need one method to do a preparation before we render and the other for some initial DOM querying / manipulation after the 1st render. But even that is exotic for an average React component. Normally, you just don't need to access the DOM manually. "And it sounds like I totally could do a check on this.state.profileName != this.props.profileName but this also seems like a hack, doesn't it?" Yes, your entire approach is a hack. And who told you componentDidReceiveProps would guarantee the props have been changed? Nothing will ever do that unless you define shouldComponentUpdate. This is just the way React works.

shea256 commented 8 years ago

@me-andre thanks for jumping in but you're being a bit too abrasive for my tastes. Also I don't believe you understood my questions. I'm quite familiar with what componentWillMount and componentDidMount do. I'll wait for @jimfb's response.

me-andre commented 8 years ago

@shea256 , "Also I don't believe you understood my questions." Could you please point at where I miss the point when I answer your questions? Also, could you please clarify what you're trying to ask. Also, we're here not to discuss personalities or tastes. This is not a private talk, neither it's a tech support. This is not even a stack exchange website where you could expect one to guide you through some area of knowledge. I often talk at local meet-ups as well as international conferences about React (and not only) and I'm very open to knowledge sharing - when and where it's appropriate. You could always contact me directly via email or skype.

Regarding your issue with loading profile data. You're trying to solve the problem of applying classic imperative approach to a functional-oriented framework. A view in React is a function of props, state and environment. Like function(state, props) { return // whatever you've computed from it } (but things get slightly more complex in real world - otherwise React wouldn't exist at all). Although in 0.14 we get pure functional components, for most components this entry function is render().

It means you start writing from render() and you assume your props are always up to date and you don't care whether props have been changed or not, how many times and where. You case could be implemented the following way:

// profile-view.js

var React = require('react');

module.exports = React.createClass({
    contextTypes: {
        app: React.PropTypes.object.isRequired
        // another option is var app = require('app')
        // or even var profiles = require('stores/profiles')
    },
    componentWillMount() {
        var {app} = this.context; // another option is to require('app')
        app.profiles.addListener('change', this.onStoreChange);
    },
    componentWillUnmount() {
        var {app} = this.context; // another option is to require('app')
        app.profiles.removeChangeListener('change', this.onStoreChange);
    },
    onStoreChange() {
        this.forceUpdate();
    },
    render() {
        var {app} = this.context;
        var profile = app.profiles.read(this.props.routeParams.id);
        if (profile) { // profile's been loaded
            return <div>{profile.name}</div>;
        } else { // not yet
            return <div>Loading...</div>;
        }
    }
});

// profiles-store.js
// app.profiles = new Profiles() on app initialization

var EventEmitter = require('events');
var {inherits} = require('util');

module.exports = Profiles;

function Profiles() {
    this.profiles = {};
}

inherits(Profiles, EventEmitter);

Profiles.prototype.read = function(id) {
    var profile = this.profiles[id];
    if (!profile) {
        $.get(`profiles/${id}`).then(profile => {
            this.profiles[id] = profile;
            this.emit('change');
        });
    }
    return profile;
};

Pretty simple. And you don't need componentDidReceiveProps. You don't even need componentWillReceiveProps and similar hooks. If you ever feel like you need them for a trivial case, you're missing understanding of how to do things in React. In order to get it please use appropriate resources, don't just trash Issues section of the repository. It feels a bit too abrasive for my tastes. It even feels like you don't respect others' time.

jimfb commented 8 years ago

@me-andre It's probably worth toning down your language a bit, as it can come across as a bit confrontational even though you're just trying to help. We want to create a welcoming community for everyone; we were all newbies at one point. Even though you are correct about some of your points on the API/design, the very fact that so many people are +1'ing the issue is an indication that something is amiss, so we should try to understand what/why people want this lifecycle. Maybe the problem is that we're not sufficiently communicating how to properly write components (documentation), or maybe React's API needs a change - either way, it's worth understanding the complaints/questions/comments here.

@shea256 this.state.profileName != this.props.profileName is checking to see if the internal state (what the component is rendering) matches what the parent is asking the component to render. This is almost the definition of "component is updating" or "component is up-to-date", so I don't see it as hacky. At least, no more hacky than the idea of "fire a data-update request when the prop changes and do a setstate in the callback" is in the first place.

@shea256 To be clear: this proposed lifecycle wouldn't enable you to do anything that you couldn't already do today with the current lifecycles. It would merely make it potentially more convenient. As others have mentioned, what you are trying to do is possible with the current lifecycles (there are multiple combinations of lifecycles that would allow you to achieve your goal). If you're trying to "make it work", then StackOverflow would be a better place to discuss. This github issue is attempting to understand the need for componentDidReceiveProps as it relates to developer ergonomics.

And who told you componentDidReceiveProps would guarantee the props have been changed?

@me-andre is correct here. Most of the lifecycle methods don't actually guarantee that something has changed; they only indicate that something might have changed. For this reason, you always need to check if previous === next if you are going to do something like make an http request. A bunch of people in this thread seem to be making the assumption that their value has changed merely because the lifecycle method fired.

@me-andre wrote:

You're trying to solve the problem of applying classic imperative approach to a functional-oriented framework.

I think this might be the root cause of people wanting this new lifecycle method, but I'm still trying to figure out why/how people want to use this method. It's entirely possible that the current lifecycle semantics are slightly miss-aligned with what people commonly want to do.

jimfb commented 8 years ago

All: Are there any use cases for componentDidReceiveProps other than "loading data asynchronously in response to a prop change"?

cc @grassick @iammerrick

grassick commented 8 years ago

@jimfb I searched my code and I also use componentWillReceiveProps to build expensive-to-create objects that are needed for render. This case suffers from the same problem that it has to pass props and be careful not to use this.props in the code.

shea256 commented 8 years ago

@jimfb wrote:

@me-andre It's probably worth toning down your language a bit, as it can come across as a bit confrontational even though you're just trying to help. We want to create a welcoming community for everyone; we were all newbies at one point. Even though you are correct about some of your points on the API/design, the very fact that so many people are +1'ing the issue is an indication that something is amiss, so we should try to understand what/why people want this lifecycle. Maybe the problem is that we're not sufficiently communicating how to properly write components (documentation), or maybe React's API needs a change - either way, it's worth understanding the complaints/questions/comments here.

Thank you for mentioning this. It's very important to make sure you have a welcoming community and my communications with you have only been pleasant.

@jimfb wrote:

@shea256 this.state.profileName != this.props.profileName is checking to see if the internal state (what the component is rendering) matches what the parent is asking the component to render. This is almost the definition of "component is updating" or "component is up-to-date", so I don't see it as hacky. At least, no more hacky than the idea of "fire a data-update request when the prop changes and do a setstate in the callback" is in the first place.

Yes, this does seem reasonable.

@jimfb wrote:

@shea256 To be clear: this proposed lifecycle wouldn't enable you to do anything that you couldn't already do today with the current lifecycles. It would merely make it potentially more convenient. As others have mentioned, what you are trying to do is possible with the current lifecycles (there are multiple combinations of lifecycles that would allow you to achieve your goal). If you're trying to "make it work", then StackOverflow would be a better place to discuss. This github issue is attempting to understand the need for componentDidReceiveProps as it relates to developer ergonomics.

I very much agree with this. I didn't post my particular use-case to receive help on it. I posted to provide some insight into why I thought componentDidReceiveProps would be useful.

I and over a dozen other people who posted clearly had an instinct to use something like this (which means that there are likely hundreds or thousands more), which indicates to me that the API is not as intuitive as it could be.

@jimfb wrote:

@me-andre is correct here. Most of the lifecycle methods don't actually guarantee that something has changed; they only indicate that something might have changed. For this reason, you always need to check if previous === next if you are going to do something like make an http request. A bunch of people in this thread seem to be making the assumption that their value has changed merely because the lifecycle method fired.

I am not making this assumption. I did leave out the check but I am now using one. But in the worst case, an additional request would be triggered.

@jimfb wrote:

I think this might be the root cause of people wanting this new lifecycle method, but I'm still trying to figure out why/how people want to use this method. It's entirely possible that the current lifecycle semantics are slightly miss-aligned with what people commonly want to do.

Perhaps. I'll think more about this.

akinnee-gl commented 8 years ago

componentDidReceiveProps would be helpful for a specific use case I have.

I'm using ReactRouter and Flux architecture. When my component is instantiated, I set my state to an item from a store. When that store emits a change event, I update my state using the same retrieval query.

When my props change because I navigated to a different item ID in the same route, I need to query my store again or my component will be stuck with state from the previous item in the store.

Currently, when componentWillReceiveProps is called, I check to see if any of my route params changed, and if they did, I call updateItemState. But, because the props have not actually changed yet, I now must pass props into my method.

this.updateItemState( nextProps );

updateItemState( props ) {
    props = props || this.props;

    this.setState( {
        item: this.getItemState( props )
    } );
}

getItemState( props ) {
    props = props || this.props;

    return this.ItemStore.get( props.params[ this.paramName ] );
}

would simply become

this.updateItemState();

updateItemState() {
    this.setState( {
        item: this.getItemState()
    } );
}

getItemState() {
    return this.ItemStore.get( this.props.params[ this.paramName ] );
}

I feel that this is a reasonable use case.

jimfb commented 8 years ago

@akinnee-gl et al: If componentDidUpdate got fired after initial mount in addition to updates, would that solve your use case?

me-andre commented 8 years ago

@akinnee-gl , if you use Flux, then you don't need to set the state from a store. Unless it's absolutely impossible, you should maintain the state in one place. When it comes to Flux that place is the store itself. When the store emits change, you just forceUpdate() and then in render() you read() your store.

"When my props change because I navigated to a different item ID in the same route, I need to query my store again or my component will be stuck with state from the previous item in the store."

Never.

Take a look: you need to render a particular item from a store and which item to render you decide from props. If that is your requirement, it should be expressed in the code in the same way as in words:

    var item = this.props.store.read(this.props.id);
    return <div>{item.name}</div>;

This is your component, nothing more. In order to make this play with Flux, you can write a reusable component for store binding/unbinding:

<FluxWrapper store={store} component={Component} id={this.props.routeParams.id} />

var FluxWrapper = React.createClass({
    componentWillMount() {
        this.props.store.addListener('change', this.onStoreChange);
    },
    componentWillUnmount() {
        this.props.store.removeListener('change', this.onStoreChange);
    },
    onStoreChange() {
        this.forceUpdate();
    },
    render() {
        var Component = this.props.component;
        return <Component {...this.props} />;
    }
});

var Component = React.createClass({
    render() {
        var item = this.props.store.read(this.props.id);
        return <div>{item.name}</div>;
    }
});

See how tiny your components can become if you use React properly.

However if your stores are heavy and you can't just read() them on every render(), then you need a caching middleware. This can be either a reusable component (similar to FluxWrapper) or an intermediate proxy store which shadows out the original read() method. Proxy stores are easy and cool - they can not only cache reads but also suppress changes if a parent store change wasn't important or significant for the case. This is called composition. You should prefer composition over inheritance or extending functionality or whatever because composition scales. As soon as you've got an issue which is hard to solve by using one component, please consider using 2 or more components instead of introducing complexity to existing components.

@jimfb , regarding my tone: I'm not a native English speaker and don't even have a regular practice of speaking, so I may sometimes pick the wrong words - I just don't feel how they sound emotionally.

akinnee-gl commented 8 years ago

Interesting approach. I will keep it in mind. All of the official docs for React say to keep your render functions pure (meaning that you should only access props and state in the render methods). All of the Flux examples show setting state from the store.