Don't count the time inside flushes towards lifecycle hooks #6860
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #6842. We keep the existing behavior of testing for matching `onBeginLifeCycleTimer`/`onEndLifeCycleTimer` calls, but we push the current timer onto the stack if we enter a flush. This solves an issue with portals which cause updates while a lifecycle timer is already running. I chose to subtract the time spent in the flush from the time counted towards the lifecycle method because it would artificially inflate the “total” time of the component due to all the components inside the portal, so it would skew the exclusive table.
| // - in the nested flush, we call it twice before and after <Foo /> rendering. (n = 3) | ||
| // - we capture the time when we exit a nested flush (n = 4) | ||
| // - we capture the time we exit componentDidMount (n = 5) | ||
| // Time spent in componentDidMount = (5 - 0 - (4 - 3)) = 4. |
There was a problem hiding this comment.
Not super happy about this. If it’s too brittle, I can probably do something nasty like call ReactDOM.render() 100 times and assert that the lifecycle own time is smaller than the inner element’s render time.
|
Sending this one your way too @spicyj 😄 |
ghost
added
| // This is how we get 4 as a number with the performanceNow() mock: | ||
| // - we capture the time we enter componentDidMount (n = 0) | ||
| // - we capture the time when we enter a nested flush (n = 1) | ||
| // - in the nested flush, we call it twice before and after <Foo /> rendering. (n = 3) |
There was a problem hiding this comment.
nit: add a comma after "twice" or else it sounds like it's called twice before and twice after
|
@gaearon updated the pull request. |
zpao
pushed a commit
to zpao/react
that referenced
this pull request
Jun 8, 2016
…#6860) * Don't count the time inside flushes towards lifecycle hooks Fixes facebook#6842. We keep the existing behavior of testing for matching `onBeginLifeCycleTimer`/`onEndLifeCycleTimer` calls, but we push the current timer onto the stack if we enter a flush. This solves an issue with portals which cause updates while a lifecycle timer is already running. I chose to subtract the time spent in the flush from the time counted towards the lifecycle method because it would artificially inflate the “total” time of the component due to all the components inside the portal, so it would skew the exclusive table. * Fix up the comment (cherry picked from commit 8d7161e)
zpao
pushed a commit
that referenced
this pull request
Jun 14, 2016
* Don't count the time inside flushes towards lifecycle hooks Fixes #6842. We keep the existing behavior of testing for matching `onBeginLifeCycleTimer`/`onEndLifeCycleTimer` calls, but we push the current timer onto the stack if we enter a flush. This solves an issue with portals which cause updates while a lifecycle timer is already running. I chose to subtract the time spent in the flush from the time counted towards the lifecycle method because it would artificially inflate the “total” time of the component due to all the components inside the portal, so it would skew the exclusive table. * Fix up the comment (cherry picked from commit 8d7161e)
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #6842.
We keep the existing behavior of testing for matching
onBeginLifeCycleTimer/onEndLifeCycleTimercalls, but we push the current timer onto the stack if we enter a flush. This solves an issue with portals which cause updates while a lifecycle timer is already running.I chose to subtract the time spent in the flush from the time counted towards the lifecycle method because it would artificially inflate the “total” time of the component due to all the components inside the portal, so it would skew the exclusive table.