Skip to content

Remove "Waiting for async callback" User Timing measurement#16379

Merged
gaearon merged 2 commits into
react:masterfrom
gaearon:no-async-callback-event
Aug 13, 2019
Merged

Remove "Waiting for async callback" User Timing measurement#16379
gaearon merged 2 commits into
react:masterfrom
gaearon:no-async-callback-event

Conversation

@gaearon

@gaearon gaearon commented Aug 13, 2019

Copy link
Copy Markdown
Collaborator

This measurement is already broken (mark/measure calls for it mismatch in Concurrent Mode). This makes debugging very annoying because "break on caught exceptions" stops there all the time.

It's difficult to keep it up to date because it models a pause rather than an actual work slice. This is why we keep breaking it. I propose that we just remove this timing, and fix it forward with Root Events.

@sizebot

sizebot commented Aug 13, 2019

Copy link
Copy Markdown
Warnings
⚠️

Base commit is broken: 21d6395

Generated by 🚫 dangerJS

@gaearon

gaearon commented Aug 13, 2019

Copy link
Copy Markdown
Collaborator Author

The test warning I added uncovered a problem in production mode which is fixed by the second commit. Note that it doesn't affect open source, but it affects Ads which uses User Timing for a subset of users in prod environment.

@bvaughn bvaughn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes debugging very annoying because "break on caught exceptions" stops there all the time.

This drives me batty.

Others might want to weigh in on this too, but I'm in favor of axing it 👍

@gaearon gaearon merged commit 1fd3906 into react:master Aug 13, 2019
@gaearon

gaearon commented Aug 13, 2019

Copy link
Copy Markdown
Collaborator Author

@acdlite said he has a WIP replacement for this on Scheduler level anyway

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants