-
Notifications
You must be signed in to change notification settings - Fork 9.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
core(user-timings): add back startTime #5442
Conversation
@@ -118,6 +118,7 @@ class UserTimings extends Audit { | |||
const time = item.isMark ? item.startTime : item.duration; | |||
return { | |||
name: item.name, | |||
startTime: item.startTime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you assert this in user-timing-test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -118,6 +118,7 @@ class UserTimings extends Audit { | |||
const time = item.isMark ? item.startTime : item.duration; | |||
return { | |||
name: item.name, | |||
startTime: item.startTime, | |||
timingType: item.isMark ? 'Mark' : 'Measure', | |||
time, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just noticed.. this name seems pretty weak. duration? timeInMs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, duration
and startTime
are better names, and they match the standard names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well that name is weak because it's either or and just for display, I added duration separately, but programmatic folks should probably just ignore time
maybe update to timeToDisplay
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's always been a bit odd that we share a column for two diff types of values
how about we add a second column, then we can be really clear with Start time
and Duration
. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add additional column? does it mess too much with our types to have undefined/null durations?
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests fail but otherwise lgtm
Related Issues/PRs
closes #5418