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

Umbrella: New Life-Cycles Ideas #7678

Closed
sebmarkbage opened this issue Sep 7, 2016 · 25 comments
Closed

Umbrella: New Life-Cycles Ideas #7678

sebmarkbage opened this issue Sep 7, 2016 · 25 comments

Comments

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Sep 7, 2016

I'll use this gather a few ideas of new life-cycle methods that help avoid existing problematic cases when using Fiber or other async rendering solutions.

  • componentDidServerRender: Gets called after the entire tree has rendered on the server. Useful for aborting buffered renders, or logging. Got the idea from Deprecate componentWillMount Maybe? #7671. This could possibly replace componentWillMount.
  • componentWasMounted and componentWasUpdated: Similar to componentDidMount and componentDidUpdate but happens asynchronously in spare cycles after a component has already appeared on the screen. That way triggering things like I/O doesn't have to block the component from rendering on the screen and dropping frames since componentDidMount have to be synchronous. This is not appropriate to use for Flux store subscriptions since you may be missing events that way. However, we can use "lazy subscriptions" as an alternative solution. componentWasMounted/componentWasUpdated (formerly componentDidDisplay) #5053.
  • componentWasUnmounted: Similar to the above, this would happen in spare cycles for clean up purposes where clean up is not synchronously needed. This may not be needed as a separate life-cycles since we can possibly just make componentWillUnmount async. Defer Execution of Unmount Life Cycle Hooks #6003.
  • componentWillMountNow and componentWillUpdateNow: This would be a fiber specific thing. Unlike the current componentWillMount / componentWillUpdate it actually happens after render and after all the children's render have been called, but right before any side-effects are committed to the DOM. The use case of reading the current scroll position in componentWillUpdate only to reset it later doesn't work with async rendering because you can scroll between componentWillUpdate and componentDidUpdate. This could possibly replace componentWillUpdate.
@ljharb
Copy link
Contributor

ljharb commented Sep 7, 2016

I'd also like to see componentWillRenderWithNewProps which always runs before every render (on client and server) where the props have changed - including the very first render (where props went from undefined to "something", conceptually) - it would run after componentWillReceiveProps and after the constructor, but before the following render.

@sebmarkbage
Copy link
Collaborator Author

componentWillReceiveProps itself is already problematic because it can be aborted and therefore those new props were never fully rendered. The ideal place to put side-effects based on new props like data fetching would be in componentWasUpdated.

If you mutated state based on new props, you could do various incorrect assumptions. Because the render could have been aborted after that so it was never visible on screen. You also don't know how often you get new props, you won't get every possible props that could've been passed down.

A slightly better API would be something like getStateBasedOnNewProps(props) where you return the new state without mutating it.

I'm not too worried about making this case easier since you can already do it, it is possible to avoid completely (ComponentKit has lived without this just fine), and using it has some footguns regardless.

@sebmarkbage
Copy link
Collaborator Author

That said, Fiber currently can call constructor, render, componentWillReceiveProps, shouldComponentUpdate followed by componentDidMount in the case that the first render was aborted and then later resumed.

@ljharb
Copy link
Contributor

ljharb commented Sep 7, 2016

I'm specifically interested in a single non-constructor method that gets called before the first render on both client and server, in case that wasn't clear - which do you recommend for that?

@sebmarkbage
Copy link
Collaborator Author

@ljharb componentWillMount if we decide to keep that for your particular use case. Otherwise, if it turns out to be a very exotic use case, I'd recommend the constructor since it is functionally the same.

@bloodyowl
Copy link
Contributor

componentDidServerRender doesn't make sense IMO. on the server you render HTML, after rendering there's nothing you're supposed to care about at the component level.

About componentWasMounted & componentWasUpdated, I believe there's no need for that. That would create more API surface, and confusion coming with it. If someone wants asynchronous calls for these events, calling setTimeout(thingToDo, 0) or requestAnimationFrame(thingToDo) does the trick without adding any new API (it also lets the user choose the most appropriate async timer behaviour, whether it's setTimeout, requestAnimationFrame or any other one).

I don't really see any use case for componentWasUnmounted. This might be interesting to make unmount unsubscriptions async, although in most cases I saw this isn't a performance bottleneck. AFAIK, making componentWillUnmount async would be tricky, as you'd be forced to keep a reference to the unmounted component in order to use its properties.

About componentWillMountNow and componentWillUpdateNow the idea might be interesting for some concepts, although for the scroll example, I'd rather like a rendering phase, then a read. Most of the time you need the exact coordinates of an element in order to scroll to it.

@sebmarkbage
Copy link
Collaborator Author

The use cases for componentDidServerRender are outlined in the linked issue.

As for the WasMounted cases. The user cannot know which is the best scheduling. Neither of those are the best since they would conflict with the next frame rendering and would block React. There is also a significant difference. React still have to do work synchronously to walk a tree and invoke those calls. It may not be the bottleneck right now but that's because we don't have Fiber yet. When we do, it can be the difference between dropping a frame and not.

I'm not sure what you mean about the scroll example. What do you mean by "rendering phase"?

@joshduck
Copy link
Contributor

joshduck commented Sep 8, 2016

I can't fathom not having a synchronous didMount method. Lots of product code still relies on reading from the DOM to set up the initial view.

@sebmarkbage
Copy link
Collaborator Author

@joshduck The proposal is not to deprecate didMount but to use it less. It is overusing it that is the problem, not the very existence. The primary synchronous use case is reading from layout and rendering again as a result. That said, async layout out of the document is something I hope we'll eventually get too.

@joshduck
Copy link
Contributor

joshduck commented Sep 8, 2016

Gotcha. I misread. You're kinda stuck in that the original componentDidMount name is not really declarative, but calling it componentWillDisplay would make it clear that it's exclusively intended for UI code.

@soupaJ
Copy link

soupaJ commented Sep 8, 2016

Just curious. Other than breaking existing code, why not change componentDidMount to get called on the server and browser instead of creating a new life-cycle method componentDidServerRender?

@sebmarkbage
Copy link
Collaborator Author

@soupaJ Good question! Breaking existing code is a major factor. It is not just an upgrade path. It is also because existing components that weren't designed for server rendering may just accidentally work. This is an important benefit to growing an ecosystem. For example, lots of npm modules weren't designed to run in a browser but they happen to work because they don't rely on anything in particular which is how things like browserify and webpack got so much use out of it. If you think about it, lots of components rely on componentDidMount for client-specific stuff and just work for server-rendering because it can be replayed.

You also need some way to differentiate server from client so that you can branch. Might as well make it explicit and branch early. It would also communicate that certain operations are not valid in componentDidServerRender such as setState.

@soupaJ
Copy link

soupaJ commented Sep 8, 2016

@sebmarkbage This is mostly what I assumed, but wanted to ask anyways :) Good point about being able to communicate when certain operations would not be valid. I didn't think about that.

@jpuri
Copy link

jpuri commented Sep 8, 2016

componentWasMounted, componentWasUnmounted and componentWillMountNow are exactly things I was looking for quite a few times while coding my views with React 👍

getStateBasedOnNewProps is also something which is used very often. But I am wondering if react always let each prop change render can that not result in extra re-rendering.

@drcmda
Copy link

drcmda commented Sep 25, 2016

I think having these would confuse the hell out of beginners, at least if they're named like this. React is already quite verbose but adding even more verbose calls with semantic differences like "was" and "did" that covey no message, address specific use-cases and have major impact behind the scenes ... React wouldn't be simple any longer.

Wouldn't it be possible to simply allow the lifecycle hooks that we already have to return promises or functions to be deferred so the internal layer can decide what's best. Functions could get postponed, promises could get rejected or canceled should the need arise (element not visible currently, etc).

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Sep 26, 2016

@drcmda I think that's a fair point. I intentionally named them with subtle semantic differences to avoid coming a up with a new naming scheme.

We will also need to leave the existing ones in for a very long time to make upgrading smooth. However, I think that some of the existing ones will become edge cases and can be relegated to "legacy".

I think we'd probably need to come up with a new naming scheme that highlights the semantic differences between the new set of preferred life-cycles. Perhaps keeping both as an upgrade path for a while.

@kof
Copy link

kof commented Dec 14, 2016

I have a use case where I need a hook which fires from timing perspective like componentWillMount but the order needs to be child first, parent second:

I need to render a style sheet for a component. Styles have source order based specificity logic. I want to be able to pass a class name rendered from the parent to the child and that class name needs to have a higher specificity than a class rendered by the child. Styles need to be always rendered before the component dom renders to avoid recalcs and repaints.

Now the current state is that a sheet rendered in the parent during componentWillMount lifecycle has a lower specificity than the one in the child. Means I can't override styles from the parent.

@trueadm
Copy link
Contributor

trueadm commented Mar 3, 2017

I'd be interested to see if we could drop the component part from the name altogether for the async versions, it looks cleaner, we no longer need to define component as the paradigm is now obvious and it means smaller bundles.

didMount() => async, throws if no Promise returned
didMountSync() => sync
componentDidMount() => deprecated, but functions as didMountSync()
didUpdate() => async, throws if no Promise returned
didUpdateSync() => sync
componentDidUpdate() => deprecated, but functions as didUpdateSync()

I'm not bothered on the actual names, I just believe making a clear distinction (a bit like Node did with async vs sync) makes a lot of sense and is a well understood pattern.

@gaearon
Copy link
Collaborator

gaearon commented Apr 18, 2017

A related problem is how do we invoke Did* methods in shallow renderer without blowing up on ref usage (#9426 (comment)). Seems like currently we don't call didMount but do call didUpdate in shallow which is quite inconsistent (correct me if I'm wrong).

@bvaughn
Copy link
Contributor

bvaughn commented Apr 18, 2017

Seems like currently we don't call didMount but do call didUpdate in shallow which is quite inconsistent (correct me if I'm wrong).

I think that's correct (see Enzyme compat tests for componentDidMount, componentDidUpdate )

Although it looks like Enzyme also has a thing called lifecycleExperimental that causes componentDidMount to be called (test here). I only mention this in passing though. I guess it's not really relevant here.

@serle
Copy link

serle commented Apr 29, 2017

Mostly I use the componentWillRecieveProps hook when I need to do something only once on a particular transition (for performance or external communication reasons) otherwise I do everything in the pure render() function. I particularly don't like the following code that has to run to test transitions of interest on each component lifecycle i.e. "if (old_prop==v1 && new_pop==v2)" ...

This got me thinking that instead of trying to name life cycle methods, an alternative could be to make the system more declarative and roll the whole thing into a subscription type pattern, with a published list of allowable transitions. What I would like to be able to do is declare that I am interested in subscribing to a particular lifecycle-state transition or change. e.g. registerLifecycleStateTransitionCallback(from_lifecycle_state, to_lifecycle_state, (state, props) => {}, intention, priority) where intention can be a list of flags like will update dom, will update state, etc that could help with the scheduling of the callback. If this is too strict and we want a looser construct, registerLifecycleStateChangeCallback(BEFORE/AFTER, lifecycle_state, (state, props) => {}, intention, priority), which is similar to the current lifecycle methods but without the proliferation of method names.

Similarly I'd like a callback for registering interest in individual property transitions e.g. registerPropertyTransitionCallback(from_prop_value, to_prop_value, (props, state) => {}, intention, priority). Again if this is too strict and we need a looser construct registerPropertyChangeCallback(BEFORE/AFTER, prop_name, (state, props) => {}, intention, priority) This could be extended to an AND specification on a set of transitions and/or property changes of interest.

@benwiley4000
Copy link

benwiley4000 commented Jun 13, 2017

On @aweary's suggestion I'm moving this idea here:

componentWillReceiveState

Only called for state updates. Should be called after componentWillReceiveProps (which may have updated the nextState), and before shouldComponentUpdate. No setState allowed.

Why? There are certain cases where it currently makes sense to run some code whenever state updates, even if a render won't happen (so componentWillUpdate won't work). E.g. a context-providing component which needs to update its child subscribers even if it's best not to re-render the immediate child due to the frequency of updates. For props we have componentWillReceiveProps. It's workable to use shouldComponentUpdate (what I'm doing now), but that's not what this lifecycle method is for.

@bvaughn
Copy link
Contributor

bvaughn commented Dec 11, 2017

componentDidServerRender now has an RFC reactjs/rfcs/pull/8 😄

@gaearon
Copy link
Collaborator

gaearon commented Mar 26, 2018

RFC for something equivalent to componentWillUpdateNow has landed.
https://github.com/reactjs/rfcs/blob/master/text/0033-new-commit-phase-lifecycles.md

@sebmarkbage
Copy link
Collaborator Author

The remaining issues are sufficiently covered by linked issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests