fix: better handle TTFB from BFCache #221
Conversation
src/observe.ts
Outdated
| // @ts-ignore | ||
| report.value = report.value - report.entries[0].requestStart; | ||
| report.value = report.entries[0] ? report.value - report.entries[0].requestStart : report.value; |
There was a problem hiding this comment.
I feel like we should just take a step forward and not report it completely.
Otherwise, it will increase the score for "good" pages. And I have a feeling people will not read the docs, and forget to skip the 0 value.
There was a problem hiding this comment.
Actually never mind, let me read this convo first. GoogleChrome/web-vitals#313
There was a problem hiding this comment.
@SuperOleg39 I read the discussion, which btw, thank you @tunetheweb for taking the time to respond with so many insights.
I was not aware of this definition "CrUX measured "TTFB"", that's brilliant.
As Perfume.js aims to be used for capturing data similar to "CrUX", let's follow that standard.
@SuperOleg39 would you mind taking care of adjusting TTFB to "CrUX" standard, adding the tests and updating the README please.
As this is a breaking changes, this means we are going to cut v9 with this fix.
There was a problem hiding this comment.
Updated onTTFB handle, will try to add some tests later, need to think about realization)
SuperOleg39
force-pushed
the
fix-bfcache-report
branch
from
6ede48c to
eb678f6
Compare
README.md
Outdated
| @@ -118,7 +118,10 @@ const perfume = new Perfume({ | |||
| myAnalyticsTool.track('storageEstimate', data); | |||
| break; | |||
| case 'TTFB': | |||
| myAnalyticsTool.track('timeToFirstByte', { duration: data }); | |||
| // Zero value mean than page restored from Back-Forward Cache, and you can ignore it before reporting | |||
| if (data > 0) { | |||
There was a problem hiding this comment.
I feel like this check should be part of Perfume at this point, no?
There was a problem hiding this comment.
Not sure about it - because of we are reported navigation type now, and 0 is not a problem when you will filter values by navigation type..
There was a problem hiding this comment.
I don't follow, what do you mean? If people should ignore 0, why reporting those values?
There was a problem hiding this comment.
Just don't have an answer, is 0 values a problem or not, and users must decide it or perfume.js internally
Because web-vitals still report this values with navigationType: 'back-forward-cache'
There was a problem hiding this comment.
Maybe ignoring 0 is a really useful default behaviour
There was a problem hiding this comment.
Good point) Will rework soon
There was a problem hiding this comment.
Zero value may mean it's 0, or it's not set (Safari reports zero for cross-origin redirect for example).
For this uncertainty reason we choose NOT to emit 0 TTFBs at all. Technically we could do it for bfcache restores (as it's by definition not a redirect and really is known to be zero), and we debated that, but in the end decided to not emit there either for consistency.
There was a problem hiding this comment.
but in the end decided to not emit there either for consistency
Don't fully understand this case - in fact, web-vitals sends 0 value in callback. Or I miss something?
There was a problem hiding this comment.
Sent fix where ignore zero
There was a problem hiding this comment.
As of this change we don't "report" a 0 TTFB: https://github.com/GoogleChrome/web-vitals/pull/281/files (see also the comments in that PR).
SuperOleg39
force-pushed
the
fix-bfcache-report
branch
from
eb678f6 to
60c5551
Compare
|
Nice! Thank you @SuperOleg39 |
|
@Zizzamia Hello! Sorry, is this fix was released? |
|
Ciao @SuperOleg39 it should under the v9 RC, I asked @lkuangPR also to help me out cut the official v9 release, as we are doing some cleanup before the official cut. Give a try to @lkuangPR can you please start working on updating the CHANGELOG with all the latest changes we are doing. This can help as well our contributors to get a sense on what's going on. |
Thanks for the answer! RC looks little bit too risky for production( |
No description provided.