-
Notifications
You must be signed in to change notification settings - Fork 47.4k
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
componentWasMounted/componentWasUpdated (formerly componentDidDisplay) #5053
Comments
Just a question: Why do we need two lifecycle methods? Why would you want a componentDidMount that does block the visual output? Just so you can apply imperative DOM manipulations before a repaint? Is that a sufficiently important use case that we care? |
I thought One issue with the name (assuming you're proposing |
@gaearon, @jimfb, yes, all the layout thrash reads have to be executed before repaint. I.e. when you render, measure, rerender to position. Otherwise you end up with an inconsistent view tree. It is equivalent of "tearing" in display technologies. It will flicker. Integrated layout would fix that but not for legacy. |
I was under the assumption this is what Would your proposal be the same as using a |
@mridgway Effectively the same, yes. Although with consistent execution order. |
cc @leebyron for API design |
Using Using |
If this path is followed, I would recommend componentDidInitialDisplay or something similar to avoid misinterpretation as something that occurs after both DidMount and DidUpdate But I'm still a little confused as to the change in lifecycle. What is the most common use case this will cover and how will the right solution become easy to discover? If continuing to use DidMount will not produce visible effects then most will probably keep doing things this way. |
Eg if this is a perf tuning tool, could it be more isolated? Maybe there is a hook similar to setTimeout that could be called for deferred nonblocking execution? |
componentDidDisplay would be a better name for a windowed list view thing. (We already have DidEnter and DidAppear for transition group though…) |
Warning might be a little weird, since the dom mutation might be behind some sort of if-check ( |
This is a perf optimization but almost all use cases should be using this new hook so something additional is too much boilerplate. Ideally no work should be done after the initial view is already done.
Additionally, just queueing these callbacks, traversing and invoking them can be significant overhead in it self (at scale). I'd prefer not requiring that, but allow React to collect them in a second pass to make the initial mount really quick. We'd not even check for the property the first pass. |
I think a lot of users assumed this is how |
The problem is that one direction leads to broken behavior and one is slower. If we change the API behavior now, we'd have to apply a codemod and potentially break everyone. |
I might be missing something here, but it sounds kind of error-prone to postpone things until after paint, possible race-conditions and what have you if you're not careful (say a store being updated between you rendering its data and subscribing to it). I'm also curious why this is being proposed, intuitively these things would normally take immeasurable time in relation to the rendering? It also seems to me that you would want to fire off network-requests as early as possible, i.e. in |
Intuitions are often wrong.
|
@sebmarkbage liked the idea . when will this be available. This feature is necessary for react |
@sebmarkbage I've mentioned it elsewhere for other purposes, but could an API like say EDIT: But of course for this use-case instead where you want to do it after display/paint. |
@syranide We would like to avoid global APIs and make them tied to a component so that we can better control scheduling such as deferring execution of offscreen content etc. It also requires at least a function call and allocating a bound callback. The point is to crazy optimize the initial render since that's the most critical one. It also encourages an imperative pattern for managing your async actions. |
I'm not sure if I agree entirely, unless you have read the documentation you will not know about or understand the intended purpose of
I agree, but
That seems petty in the context of instancing all the components, rendering all the components, setting up the DOM and then drawing the DOM. Nor is every component is going to need to use it. ... If all we want is |
Could Then there is only one place to document, rather than having 'for this case use |
A |
This is basically what |
Proposal: New life-cycle that fires after
componentDidMount
and after the event-loop has returned but before any other reconciliation has started.Effectively, this give you a
componentDidMount
that doesn't block the visual output. It could be a good place to set up timers, subscriptions etc.The text was updated successfully, but these errors were encountered: