fix(metrics): Check for cls entry sources #3775
Conversation
The LayoutShift.sources field may not exist in certain browsers, even if the LayoutShift API is there. This PR patches logic that adds CLS data to make sure that `._clsEntry.sources` exists. Ref: https://developer.mozilla.org/en-US/docs/Web/API/LayoutShift
| @@ -214,7 +214,8 @@ export class MetricsInstrumentation { | |||
| transaction.setTag('lcp.size', this._lcpEntry.size); | |||
| } | |||
|
|
|||
| if (this._clsEntry) { | |||
| // See: https://developer.mozilla.org/en-US/docs/Web/API/LayoutShift | |||
| if (this._clsEntry && this._clsEntry.sources) { | |||
There was a problem hiding this comment.
| if (this._clsEntry && this._clsEntry.sources) { | |
| if (this._clsEntry?.sources) { |
The tiniest of nits. I just really like optional chaining. 😛
There was a problem hiding this comment.
Not sure it's worth the extra code generated from typescript: https://www.typescriptlang.org/play?ts=3.7.5#code/MYewdgzgLgBFAWAuGBvGB9YAbCBRMUATgJ4D8yaEIArocAKYTkzSECWYA5gNoC6MAX0EwAvKgEBYAFDSA9LJgB5AA5Q24AIZYYAYXgaOHTtLYAzGAAoEAOkw58RMtaq0GEAJTjpchQCV6nNRYGoQwoGAAJmxq4CbmVvC22HgEJDAAZOlwiXYpjs40dIyeKJIyUkA, especially if every byte counts.
Thought that is why we shied away from optional chaining outside of tests?
There was a problem hiding this comment.
We kinda started to use it out of convenience tbh. It's a shame that only ES2020 benefits from its smaller footprint, because we don't target it yet. I don't like it, but we might want to verify what's the impact of it and if it's unreasonable, us eslint to prevent usage :(
There was a problem hiding this comment.
(with/without chaining)
size-limit report
|
There was a problem hiding this comment.
Testing this specifically might not be worth the effort, but this change together with other recent changes to web vitals suggest we could be more defensive and have try...catch around potentially breaking code. The outcome would be silencing and perhaps taking more time to find problems like this one, so I'm not 100% sure that'd be a good idea. Thoughts?
Tbh I think it'd not be silencing anything, as this API is just not available on some envs anyway. It might silence some capturing code down the call stack though. |
If done at an inappropriate level of granularity, it could accidentally silence SDK bugs. We would have no visibility into how many times a piece of instrumentation did its job vs threw an exception and moved on. |
|
That's what I said in second sentence :P |
The LayoutShift.sources field may not exist in certain browsers, even if
the LayoutShift API is there. This PR patches logic that adds CLS data
to make sure that
._clsEntry.sourcesexists.Ref: https://developer.mozilla.org/en-US/docs/Web/API/LayoutShift
See Sentry issue: https://sentry.io/share/issue/ebd52ddcc2c64a91b61e7a5486ec71bc/