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

componentWasMounted/componentWasUpdated (formerly componentDidDisplay) #5053

Closed
sebmarkbage opened this issue Oct 5, 2015 · 23 comments
Closed
Labels

Comments

@sebmarkbage
Copy link
Collaborator

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.

@jimfb
Copy link
Contributor

jimfb commented Oct 5, 2015

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?

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2015

I thought componentDidMount runs from children to parent so by the time it runs, everything is mounted (and displayed). Am I missing the point?

One issue with the name (assuming you're proposing componentDidRender and it's not just a placeholder name) is that beginners might assume componentDidRender runs after every render and is effectively componentDidUpdate.

@sebmarkbage
Copy link
Collaborator Author

@gaearon, componentDidDisplay maybe.

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

@mridgway
Copy link
Contributor

mridgway commented Oct 5, 2015

I was under the assumption this is what componentDidMount was doing as well. It seems like most people are using componentDidMount for the things you mentioned already. Is the logic for componentDidMount changing or was it just misunderstood?

Would your proposal be the same as using a setTimeout inside of componentDidMount?

@sebmarkbage
Copy link
Collaborator Author

@mridgway Effectively the same, yes. Although with consistent execution order.

@sebmarkbage sebmarkbage changed the title componentDidRender componentDidDisplay Oct 5, 2015
@sebmarkbage
Copy link
Collaborator Author

cc @leebyron for API design

@sebmarkbage
Copy link
Collaborator Author

Using componentDidDisplay when you should use componentDidMount will show itself as flickering so I'm not too worried about that.

Using componentDidMount when you should've used componentDidDisplay is trickier. We could potentially catch componentDidMounts that doesn't mutate the DOM in anyway (by refs or second render pass) and warn for those.

@leebyron
Copy link
Contributor

leebyron commented Oct 5, 2015

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.

@leebyron
Copy link
Contributor

leebyron commented Oct 5, 2015

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?

@sophiebits
Copy link
Collaborator

componentDidDisplay would be a better name for a windowed list view thing. (We already have DidEnter and DidAppear for transition group though…)

@jimfb
Copy link
Contributor

jimfb commented Oct 5, 2015

Using componentDidMount when you should've used componentDidDisplay is trickier. We could potentially catch componentDidMounts that doesn't mutate the DOM in anyway (by refs or second render pass) and warn for those.

Warning might be a little weird, since the dom mutation might be behind some sort of if-check (if(refComponentIsTooLarge()) { moveChildIntoOtherBox(); }).

@sebmarkbage
Copy link
Collaborator Author

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.

setTimeout (or something like setImmediate) might not be batched properly and is difficult to reason about with regard to other things that call them. We also can't schedule them appropriately when we have a scheduled reconciler.

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.

@mridgway
Copy link
Contributor

mridgway commented Oct 5, 2015

I think a lot of users assumed this is how componentDidMount was working to begin with and as you said, most of the current uses of the method should be using your newly proposed API. Should the behavior of componentDidMount be changed and introduce a new lifecycle method for the current implementation instead? componentWillMount is unfortunately already taken but IMO makes the most sense for the current behavior of componentDidMount.

@sebmarkbage
Copy link
Collaborator Author

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.

@syranide
Copy link
Contributor

syranide commented Oct 6, 2015

componentDidPaint sounds like it could make more sense perhaps? Display sounds like it's part of being displayed/hidden (i.e. like the CSS-property).

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 componentWillMount rather than later. I'd rather delay painting by 1ms than a network request by 100ms. So I'm curious, do you have use-cases where there would be measurable benefits? (For which setTimeout would not be sufficient).

@sebmarkbage
Copy link
Collaborator Author

Intuitions are often wrong.

On Oct 6, 2015, at 2:43 AM, Andreas Svensson notifications@github.com wrote:

componentDidPaint sounds like it could make more sense perhaps? Display sounds like it's part of being displayed/hidden (i.e. like the CSS-property).

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. 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 componentWillMount rather than later. I'd rather delay painting by 1ms than the network request by 100ms. So I'm curious, do you have use-cases where there would be definite benefits of this? (For which setTimeout would not be sufficient).


Reply to this email directly or view it on GitHub.

@TonyFrancis
Copy link

@sebmarkbage liked the idea . when will this be available. This feature is necessary for react

@syranide
Copy link
Contributor

@sebmarkbage I've mentioned it elsewhere for other purposes, but could an API like say React.defer(callback) be more versatile and intuitive perhaps? It would queue all callbacks and execute when React is finished with all processing (i.e. after events, transactions, etc).

EDIT: But of course for this use-case instead where you want to do it after display/paint.

@sebmarkbage
Copy link
Collaborator Author

@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. this.defer(callback) would work but that lacks the pit-of-success factor. You have to do something extra and more work to do something that is best practice and you always should do.

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.

@syranide
Copy link
Contributor

this.defer(callback) would work but that lacks the pit-of-success factor.

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 componentDidDisplay. setTimeout(cb)/setImmediate(cb) is a very common pattern in JS for deferring.

You have to do something extra and more work to do something that is best practice and you always should do.

I agree, but componentDidDisplay only works for mount and not updates. Isn't that worth supporting too? Or is this only about the initial subscription to stores basically? And shouldn't we have an equivalent for unmount too since we shouldn't want to block there either? That seems equally important for page switches.

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.

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 componentDidDisplay then it makes sense to go that way, but it seems like a partial solution?

@RoyalIcing
Copy link

Could componentDidMount allow a function to be returned that gets deferred?

Then there is only one place to document, rather than having 'for this case use componentDidMount, for these situations use componentDidDisplay' which could make things confusing. Also by returning it makes it obvious that this will be after the stuff in componentDidMount has completed.

@sebmarkbage sebmarkbage changed the title componentDidDisplay componentWasMounted/componentWasUpdated (formerly componentDidDisplay) Sep 7, 2016
@bloodyowl
Copy link
Contributor

A componentDidRender lifecycle method would be interesting, for browser|native|* specific patches 👍

@necolas necolas added the React Core Team Opened by a member of the React Core Team label Jan 8, 2020
@gaearon
Copy link
Collaborator

gaearon commented Mar 24, 2021

This is basically what useEffect is.

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

No branches or pull requests