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

Clarify lifecycle (renderComponent vs. renderComponentToString) #968

Closed
matthewwithanm opened this issue Jan 24, 2014 · 8 comments
Closed

Comments

@matthewwithanm
Copy link
Contributor

As discussed on IRC, the lifecycle functions are misleading when using renderComponentToString. Namely, because componentWillMount is called, but componentDidMount is not.

At the least, this should be documented on the lifecycle docs page. There was also discussion about changing the method names. (Note: there's clearly a need for both hooks; the only issue is that name of the former conveys that the component will mount when, in fact, it won't.)

One possibility is to add another always-run phase before the mounting phase which is run when rendered to string or DOM (e.g. "prepare" or "initialize"). The lifecycle would then be easily explained as follows:

  1. Prepare the compoent
  2. Mount the component (if rendering to DOM)
  3. Render the component for the first time.

I'd be happy to contribute a PR for either docs, method names, or both once a decision is reached.

@petehunt
Copy link
Contributor

I think we could probably get away with not calling componentWillMount() when server rendering. Thoughts anyone? @zpao @jordwalke @sebmarkbage @spicyj ?

@syranide
Copy link
Contributor

@petehunt The only issue I see with that is that it breaks the expectation of getInitialState > componentWillMount > render: I.e. you now explicitly prevent changes to state from componentWillMount, and if you can't change the state then what is it actually useful for? You can set up timers, requests, eventbus subscribing, etc, but DidMount works equally well for that today (not sure what the long-term plan is). My ReactSWF currently relies on this, althought it's a bit of a unique case, and accessing props from getInitialState doesn't feel clean (but perhaps it is).

@sebmarkbage
Copy link
Collaborator

One possible solution is to actually replace componentWillMount with the constructor. Since the descriptor change, the constructor and componentWillMount is called at the same time. It also clarifies why there is no componentDidUnmount.

@syranide
Copy link
Contributor

@sebmarkbage Sounds reasonable, my only spontaneous reaction is that I really like the obvious nature of componentWillMount > componentDidMount > ... > componentWillUnmount whereas using the constructor instead removes the obviousness of it all as it becomes constructor > componentDidMount > ... > componentWillUnmount.

And in-terms of getInitialState I intuitively see it as getInitialState -> componentWillMount -> componentDidMount and not constructor -> getInitialState -> componentDidMount.

@sebmarkbage
Copy link
Collaborator

The idea is to replace getInitialState with a property initializer, which doesn't currently exist in ES6 but we're proposing it for ES7 (similar to TypeScript). The idea is to follow the future idiomatic way to handle initialization.

class Foo {
  state = { counter: 0 };
  constructor() {
    this.state.counter; // === 0
  }
}

If a destruction primitive like .NET's dispose() were ever introduced, that would correspond to a componentDidUnmount.

The mental model is that you just follow the normal initialization process of a class. We just add the notion of mount/unmount.

class Foo {
  state = { counter: 0 };
  constructor() {
    this.state.counter; // === 0
  }
  didMount() {
  }
  willUpdate() {
  }
  didUpdate() {
  }
  willUnmount() {
  }
}

Since there's no distinction between did and will in this model, we might be able to just shorten it to mount/unmount but that probably creates other problems such as what to do with will/didUpdate.

@syranide
Copy link
Contributor

@sebmarkbage Ah that makes a lot of sense.

PS. I'm preally impressed with the overall discussion that is going on now with the API, exciting stuff!

@matthewwithanm
Copy link
Contributor Author

This may deserve its own thread, but it's related to @sebmarkbage's comments so I figured I'd post here.

The fact that componentWillMount is (currently) the de facto constructor seems to get in the way of potential asynchronous rendering since it's (by definition) called as part of the (now synchronous) rendering process. I agree with the decision to switch renderComponentToString to the current synchronous signature (FWIW), but that means stepping outside of React to do stuff before rendering. For example:

var c = MyComponent({
  onReady: function() {
    var rendered = React.renderComponentToString(c);
    // Do whatever with it
  }
}).initialize();

This illustrates how you might create a custom initializer and invoke it manually, since the de facto initializer—componentWillMount—isn't called until after we try to render the component.

It seems to me there's a pretty strong argument for this kind of initialization (decoupled from synchronous rendering) to be formalized as part of the life cycle.

@sophiebits
Copy link
Collaborator

We do want to support async server rendering, but I'm going to close this issue out as the original docs it asked for are done now and I think we have other issues open to track async server rendering.

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

5 participants