Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify mount and update component life-cycle methods #2074

Closed
syranide opened this issue Aug 20, 2014 · 7 comments
Closed

Unify mount and update component life-cycle methods #2074

syranide opened this issue Aug 20, 2014 · 7 comments

Comments

@syranide
Copy link
Contributor

Kind of related #2073

It seems to me there's a natural correlation between mount and update methods:

getInitialState    => componentWillReceiveProps
(always true)      => shouldComponentUpdate
componentWillMount => componentWillUpdate
componentDidMount  => componentDidUpdate

In many situations you want to have the same code for mount/update, but you have to put it in a shared method and call it from both places. It's hardly common enough that it's an issue, but it seems common enough that it strikes me as a bit odd... perhaps. Just an idea, could it make sense to e.g. get rid of mount altogether and instead have the following (it's extreme perhaps, but just as an example):

// mounting
getInitialState
componentWillReceiveProps(update = false?)
componentWillUpdate(update = false)
componentDidUpdate(update = false)

// updating
componentWillReceiveProps(update = true?)
shouldComponentUpdate
componentWillUpdate(update = true)
componentDidUpdate(update = true)

It would unify the life-cycle and move away from mounting being treated as entirely separate from updating, it would then be seen as an exception. There would be less potential for inconsistencies causes by mistake/neglect.

I haven't thought this through extensively and bring it up mainly to start a discussion, I know many others have wondered why there isn't a life-cycle method for mount+update.

@oriSomething
Copy link

for me unify componentDidMount with componentDidUpdate it's problematic because i need it to some jQuery plugins that i still don't have the time write fully as React component or… for loading data from the server.
i understand the duplication problem, but for me it's rare to write one function that is called from the methods alike. it's easier to have the more functions

@justinwoo
Copy link
Contributor

+1 @oriSomething
As ugly as it is, having the distinction between componentDidMount and componentDidUpdate really helps me "manage" DOM nodes for things like jQuery plugins. This concept isn't foreign to other frameworks either -- even Ember comes with event methods like didInsertElement to help people manage their nodes separate from the object update cycle.

In addition, isn't doing addChangeListener to stores on componentDidMount the norm now for Flux applications? Do you want people to manage these component mount/unmount actions separately?

@syranide
Copy link
Contributor Author

I'm not saying you're wrong, but at the heart, React works much like old-school server-rendered pages. Render page, click something, throw everything away, render page. In this model there is no separation between first and subsequent renders. Now that's obviously not exactly how React works in practice due to reconciliation, so there are obviously a need for mount and unmount.

But my curiosity lies in that my "proposed" change makes inconsistent/broken behavior less likely to occur as only listening to mount is a special-case, rather than the other way around:

React.createClass({
  componentDidUpdate: function(first) {
    // suboptimal (but not necessarily broken)
    addEventListener(...);
  }
});

React.createClass({
  componentDidUpdate: function(first) {
    if (first) {
      // optimal
      addEventListener(...);
    }
  }
});

React.createClass({
  componentDidUpdate: function(first) {
    // optimal
    measureDOM();
  }
});

Whereas with the old separation you can "easily" have inconsistencies:

React.createClass({
  componentDidMount: function() {
    // optimal
    addEventListener(...);
  }
});

React.createClass({
  componentDidMount: function() {
    // inconsistent / broken
    measureDOM();
  }
});

React.createClass({
  componentDidMount: function() {
    // optimal / correct
    measureDOM();
  },
  componentDidUpdate: function() {
    // once more
    measureDOM();
  }
});

I also think my proposed change makes it easier to understand how a component is implemented, as you no longer need to correlate the differences of mount and update.

PS. Again, I'm not forcefully pushing for this, but I think it's an interesting topic.

@justinwoo
Copy link
Contributor

Overall I agree with you, and honestly, there's something like a 50/50 split for me between having actually separate mount/update behavior and a shared method being called for both (outside of jQuery plugin wrapping).

I think it'd be nice to try this idea out and see how it might work out for use cases that involve forcefully manipulating the DOM element. IIRC Ember components just do both where the mount methods are called and then the update method runs directly after them. I think doing this makes sense, but maybe it should be conditional definitions of mount methods that decides whether or not mount or update methods are called on component render.

@mnpenner
Copy link
Contributor

👍 for combining the methods and having update as an argument. This means less repetition, and anyone who needs the separation can just check the arg.

@J-F-Liu
Copy link

J-F-Liu commented Mar 9, 2016

I suggest React also run componentWillReceiveProps in mounting life-cycle, so we can put the same code for mount/update inside componentWillReceiveProps.

Usually this code does async data fetching according to a value given in props and change state after data is fetched. So both componentDidMount and componentDidUpdate are not a good place to do this.

@gaearon
Copy link
Collaborator

gaearon commented Oct 27, 2016

We might consider this in the future. Let's continue the discussion in #7678.

@gaearon gaearon closed this as completed Oct 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants