Skip to content
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

fix: safe performance.getEntriesByType #5849

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Conversation

mxdvl
Copy link
Contributor

@mxdvl mxdvl commented Aug 25, 2022

What does this change?

Some browsers do not support window.performance.getEntriesByType, so we check for its existence before measuring the time taken.

Why?

This is the top reported TypeError in Sentry.

Fixes CLIENT-SIDE-PROD-CSXR
Fixes CLIENT-SIDE-PROD-CTRH

This method was Introduced in #5298.
Related to the fix in #5299

@mxdvl mxdvl added the Health label Aug 25, 2022
@mxdvl mxdvl requested a review from a team as a code owner August 25, 2022 08:53
Copy link
Contributor

@bryophyta bryophyta left a comment

Choose a reason for hiding this comment

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

great catch!

@mxdvl mxdvl changed the title fix: stop performance.getEntriesByType fix: safe performance.getEntriesByType Aug 25, 2022
Some browsers do not support this method, so we check for
its existence before measuring the time taken.

Fixes CLIENT-SIDE-PROD-CSXR
Fixes CLIENT-SIDE-PROD-CTRH
@mxdvl mxdvl force-pushed the mxdvl/safe-get-entries-by-type branch from 4370130 to a64ff08 Compare August 25, 2022 08:57
@bryophyta
Copy link
Contributor

I guess there are a few non-equivalent ways of checking whether an object has a property/method in js, right? I'm never clear on exactly which to use when. in looks to me like it should work here, but I'd be interested to know if you've got any guidelines on when to use what test?

@github-actions
Copy link

github-actions bot commented Aug 25, 2022

Size Change: +25 B (0%)

Total Size: 1.65 MB

ℹ️ View Unchanged
Filename Size Change
dotcom-rendering/dist/1382.legacy.********************.js 5.67 kB 0 B
dotcom-rendering/dist/1794.legacy.********************.js 3.31 kB 0 B
dotcom-rendering/dist/1815.legacy.********************.js 30.9 kB 0 B
dotcom-rendering/dist/2344.legacy.********************.js 2.78 kB 0 B
dotcom-rendering/dist/259.legacy.********************.js 3.25 kB 0 B
dotcom-rendering/dist/286.********************.js 23.2 kB 0 B
dotcom-rendering/dist/286.legacy.********************.js 23.9 kB 0 B
dotcom-rendering/dist/2947.********************.js 2.74 kB 0 B
dotcom-rendering/dist/2947.legacy.********************.js 2.82 kB 0 B
dotcom-rendering/dist/2959.legacy.********************.js 4.92 kB 0 B
dotcom-rendering/dist/3210.********************.js 8.57 kB 0 B
dotcom-rendering/dist/3307.********************.js 3.12 kB 0 B
dotcom-rendering/dist/3584.********************.js 1.8 kB 0 B
dotcom-rendering/dist/3584.legacy.********************.js 1.8 kB 0 B
dotcom-rendering/dist/4019.********************.js 3.44 kB 0 B
dotcom-rendering/dist/4019.legacy.********************.js 3.48 kB 0 B
dotcom-rendering/dist/4869.legacy.********************.js 6.68 kB 0 B
dotcom-rendering/dist/5058.********************.js 2.73 kB 0 B
dotcom-rendering/dist/5436.legacy.********************.js 2.68 kB 0 B
dotcom-rendering/dist/5553.********************.js 2.87 kB 0 B
dotcom-rendering/dist/5887.********************.js 5.94 kB 0 B
dotcom-rendering/dist/6101.********************.js 32.9 kB 0 B
dotcom-rendering/dist/6131.********************.js 4.29 kB 0 B
dotcom-rendering/dist/6131.legacy.********************.js 4.3 kB 0 B
dotcom-rendering/dist/6204.********************.js 5.54 kB 0 B
dotcom-rendering/dist/6400.********************.js 21.5 kB 0 B
dotcom-rendering/dist/6400.legacy.********************.js 21.5 kB 0 B
dotcom-rendering/dist/6959.legacy.********************.js 6.46 kB 0 B
dotcom-rendering/dist/7137.legacy.********************.js 6.4 kB 0 B
dotcom-rendering/dist/7148.********************.js 6.8 kB 0 B
dotcom-rendering/dist/7148.legacy.********************.js 7.31 kB 0 B
dotcom-rendering/dist/7576.********************.js 3.96 kB 0 B
dotcom-rendering/dist/7576.legacy.********************.js 5.38 kB 0 B
dotcom-rendering/dist/7800.********************.js 11.3 kB 0 B
dotcom-rendering/dist/7829.********************.js 2.8 kB 0 B
dotcom-rendering/dist/7829.legacy.********************.js 2.91 kB 0 B
dotcom-rendering/dist/7981.********************.js 4.32 kB 0 B
dotcom-rendering/dist/8129.legacy.********************.js 11.8 kB 0 B
dotcom-rendering/dist/8195.legacy.********************.js 2.98 kB 0 B
dotcom-rendering/dist/8414.********************.js 4.41 kB 0 B
dotcom-rendering/dist/8414.legacy.********************.js 4.49 kB 0 B
dotcom-rendering/dist/8471.legacy.********************.js 5.45 kB 0 B
dotcom-rendering/dist/8492.legacy.********************.js 4.41 kB 0 B
dotcom-rendering/dist/8612.********************.js 3.69 kB 0 B
dotcom-rendering/dist/8612.legacy.********************.js 4.18 kB 0 B
dotcom-rendering/dist/8808.********************.js 5.3 kB 0 B
dotcom-rendering/dist/9380.legacy.********************.js 2.83 kB 0 B
dotcom-rendering/dist/9748.********************.js 4.74 kB 0 B
dotcom-rendering/dist/AlreadyVisited-importable.********************.js 4.52 kB 0 B
dotcom-rendering/dist/AlreadyVisited-importable.legacy.********************.js 4.52 kB 0 B
dotcom-rendering/dist/atomIframe.********************.js 757 B 0 B
dotcom-rendering/dist/atomIframe.legacy.********************.js 806 B 0 B
dotcom-rendering/dist/AudioAtomWrapper-importable.********************.js 459 B 0 B
dotcom-rendering/dist/AudioAtomWrapper-importable.legacy.********************.js 517 B 0 B
dotcom-rendering/dist/bootCmp.********************.js 9.64 kB 0 B
dotcom-rendering/dist/bootCmp.legacy.********************.js 13.3 kB 0 B
dotcom-rendering/dist/Branding-importable.********************.js 4.65 kB 0 B
dotcom-rendering/dist/Branding-importable.legacy.********************.js 4.65 kB 0 B
dotcom-rendering/dist/braze-web-sdk-core.********************.js 36.1 kB 0 B
dotcom-rendering/dist/braze-web-sdk-core.legacy.********************.js 36.1 kB 0 B
dotcom-rendering/dist/BrazeMessaging-importable.********************.js 9.44 kB 0 B
dotcom-rendering/dist/BrazeMessaging-importable.legacy.********************.js 10.2 kB 0 B
dotcom-rendering/dist/CalloutBlockComponent-importable.********************.js 4.26 kB 0 B
dotcom-rendering/dist/CalloutBlockComponent-importable.legacy.********************.js 4.55 kB 0 B
dotcom-rendering/dist/Carousel-importable.********************.js 11.5 kB 0 B
dotcom-rendering/dist/Carousel-importable.legacy.********************.js 12 kB 0 B
dotcom-rendering/dist/ChartAtomWrapper-importable.********************.js 261 B 0 B
dotcom-rendering/dist/ChartAtomWrapper-importable.legacy.********************.js 271 B 0 B
dotcom-rendering/dist/CommentCount-importable.********************.js 3.46 kB 0 B
dotcom-rendering/dist/CommentCount-importable.legacy.********************.js 3.56 kB 0 B
dotcom-rendering/dist/CommercialMetrics-importable.********************.js 6.47 kB 0 B
dotcom-rendering/dist/CommercialMetrics-importable.legacy.********************.js 810 B 0 B
dotcom-rendering/dist/CoreVitals-importable.********************.js 6.09 kB 0 B
dotcom-rendering/dist/CoreVitals-importable.legacy.********************.js 6.31 kB 0 B
dotcom-rendering/dist/debug.js 1.75 kB 0 B
dotcom-rendering/dist/DiscussionContainer-importable.********************.js 3.5 kB 0 B
dotcom-rendering/dist/DiscussionContainer-importable.legacy.********************.js 3.75 kB 0 B
dotcom-rendering/dist/DiscussionMeta-importable.********************.js 3.94 kB 0 B
dotcom-rendering/dist/DiscussionMeta-importable.legacy.********************.js 4.05 kB 0 B
dotcom-rendering/dist/DocumentBlockComponent-importable.********************.js 2.99 kB 0 B
dotcom-rendering/dist/DocumentBlockComponent-importable.legacy.********************.js 3.1 kB 0 B
dotcom-rendering/dist/dynamicImport.********************.js 2 kB 0 B
dotcom-rendering/dist/dynamicImport.legacy.********************.js 2.07 kB 0 B
dotcom-rendering/dist/EditionDropdown-importable.********************.js 4.1 kB 0 B
dotcom-rendering/dist/EditionDropdown-importable.legacy.********************.js 4.15 kB 0 B
dotcom-rendering/dist/EmbedBlockComponent-importable.********************.js 3.34 kB 0 B
dotcom-rendering/dist/EmbedBlockComponent-importable.legacy.********************.js 3.45 kB 0 B
dotcom-rendering/dist/embedIframe.********************.js 759 B 0 B
dotcom-rendering/dist/embedIframe.legacy.********************.js 810 B 0 B
dotcom-rendering/dist/EnhancePinnedPost-importable.********************.js 6.24 kB 0 B
dotcom-rendering/dist/EnhancePinnedPost-importable.legacy.********************.js 6.82 kB 0 B
dotcom-rendering/dist/FetchCommentCounts-importable.********************.js 1.73 kB 0 B
dotcom-rendering/dist/FetchCommentCounts-importable.legacy.********************.js 1.78 kB 0 B
dotcom-rendering/dist/FetchOnwardsData-importable.********************.js 2.23 kB 0 B
dotcom-rendering/dist/FetchOnwardsData-importable.legacy.********************.js 2.23 kB 0 B
dotcom-rendering/dist/FilterButton-importable.********************.js 2.64 kB 0 B
dotcom-rendering/dist/FilterButton-importable.legacy.********************.js 2.74 kB 0 B
dotcom-rendering/dist/FilterKeyEventsToggle-importable.********************.js 3.82 kB 0 B
dotcom-rendering/dist/FilterKeyEventsToggle-importable.legacy.********************.js 3.89 kB 0 B
dotcom-rendering/dist/FocusStyles-importable.********************.js 4.71 kB 0 B
dotcom-rendering/dist/FocusStyles-importable.legacy.********************.js 4.77 kB 0 B
dotcom-rendering/dist/frontend.server.js 490 kB +12 B (0%)
dotcom-rendering/dist/ga.********************.js 2.83 kB 0 B
dotcom-rendering/dist/ga.legacy.********************.js 2.9 kB 0 B
dotcom-rendering/dist/GetCricketScoreboard-importable.********************.js 4.01 kB 0 B
dotcom-rendering/dist/GetCricketScoreboard-importable.legacy.********************.js 4.19 kB 0 B
dotcom-rendering/dist/GetMatchNav-importable.********************.js 7.32 kB 0 B
dotcom-rendering/dist/GetMatchNav-importable.legacy.********************.js 7.46 kB 0 B
dotcom-rendering/dist/GetMatchStats-importable.********************.js 6.36 kB 0 B
dotcom-rendering/dist/GetMatchStats-importable.legacy.********************.js 7.14 kB 0 B
dotcom-rendering/dist/GetMatchTabs-importable.********************.js 3.08 kB 0 B
dotcom-rendering/dist/GetMatchTabs-importable.legacy.********************.js 3.24 kB 0 B
dotcom-rendering/dist/guardian-braze-components-banner.********************.js 11.3 kB 0 B
dotcom-rendering/dist/guardian-braze-components-banner.js 10.3 kB 0 B
dotcom-rendering/dist/guardian-braze-components-banner.legacy.********************.js 11.4 kB 0 B
dotcom-rendering/dist/guardian-braze-components-end-of-article.********************.js 9.28 kB 0 B
dotcom-rendering/dist/guardian-braze-components-end-of-article.js 9.75 kB 0 B
dotcom-rendering/dist/guardian-braze-components-end-of-article.legacy.********************.js 9.36 kB 0 B
dotcom-rendering/dist/GuideAtomWrapper-importable.********************.js 263 B 0 B
dotcom-rendering/dist/GuideAtomWrapper-importable.legacy.********************.js 273 B 0 B
dotcom-rendering/dist/initDiscussion.********************.js 11 kB 0 B
dotcom-rendering/dist/initDiscussion.legacy.********************.js 11.4 kB 0 B
dotcom-rendering/dist/InstagramBlockComponent-importable.********************.js 3 kB 0 B
dotcom-rendering/dist/InstagramBlockComponent-importable.legacy.********************.js 3.09 kB 0 B
dotcom-rendering/dist/InteractiveBlockComponent-importable.********************.js 4.26 kB 0 B
dotcom-rendering/dist/InteractiveBlockComponent-importable.legacy.********************.js 4.43 kB 0 B
dotcom-rendering/dist/islands.********************.js 11.2 kB 0 B
dotcom-rendering/dist/islands.legacy.********************.js 12.1 kB +13 B (0%)
dotcom-rendering/dist/KeyEventsCarousel-importable.********************.js 2.9 kB 0 B
dotcom-rendering/dist/KeyEventsCarousel-importable.legacy.********************.js 3.01 kB 0 B
dotcom-rendering/dist/KnowledgeQuizAtomWrapper-importable.********************.js 268 B 0 B
dotcom-rendering/dist/KnowledgeQuizAtomWrapper-importable.legacy.********************.js 280 B 0 B
dotcom-rendering/dist/LabsHeader-importable.********************.js 8.66 kB 0 B
dotcom-rendering/dist/LabsHeader-importable.legacy.********************.js 8.79 kB 0 B
dotcom-rendering/dist/Links-importable.********************.js 6.33 kB 0 B
dotcom-rendering/dist/Links-importable.legacy.********************.js 7.39 kB 0 B
dotcom-rendering/dist/LiveBlogEpic-importable.********************.js 6.22 kB 0 B
dotcom-rendering/dist/LiveBlogEpic-importable.legacy.********************.js 6.9 kB 0 B
dotcom-rendering/dist/Liveness-importable.********************.js 3.58 kB 0 B
dotcom-rendering/dist/Liveness-importable.legacy.********************.js 3.65 kB 0 B
dotcom-rendering/dist/MapEmbedBlockComponent-importable.********************.js 5.11 kB 0 B
dotcom-rendering/dist/MapEmbedBlockComponent-importable.legacy.********************.js 5.34 kB 0 B
dotcom-rendering/dist/MostViewedFooterData-importable.********************.js 7.6 kB 0 B
dotcom-rendering/dist/MostViewedFooterData-importable.legacy.********************.js 7.75 kB 0 B
dotcom-rendering/dist/MostViewedRightWrapper-importable.********************.js 6.99 kB 0 B
dotcom-rendering/dist/MostViewedRightWrapper-importable.legacy.********************.js 4.95 kB 0 B
dotcom-rendering/dist/newsletterEmbedIframe.********************.js 931 B 0 B
dotcom-rendering/dist/newsletterEmbedIframe.legacy.********************.js 975 B 0 B
dotcom-rendering/dist/OnwardsUpper-importable.********************.js 6.79 kB 0 B
dotcom-rendering/dist/OnwardsUpper-importable.legacy.********************.js 7 kB 0 B
dotcom-rendering/dist/ophan.********************.js 7.24 kB 0 B
dotcom-rendering/dist/ophan.legacy.********************.js 7.86 kB 0 B
dotcom-rendering/dist/PersonalityQuizAtomWrapper-importable.********************.js 271 B 0 B
dotcom-rendering/dist/PersonalityQuizAtomWrapper-importable.legacy.********************.js 282 B 0 B
dotcom-rendering/dist/ProfileAtomWrapper-importable.********************.js 264 B 0 B
dotcom-rendering/dist/ProfileAtomWrapper-importable.legacy.********************.js 274 B 0 B
dotcom-rendering/dist/PulsingDot-importable.********************.js 1.66 kB 0 B
dotcom-rendering/dist/PulsingDot-importable.legacy.********************.js 1.76 kB 0 B
dotcom-rendering/dist/QandaAtomWrapper-importable.********************.js 262 B 0 B
dotcom-rendering/dist/QandaAtomWrapper-importable.legacy.********************.js 272 B 0 B
dotcom-rendering/dist/ReaderRevenueDev-importable.********************.js 4.58 kB 0 B
dotcom-rendering/dist/ReaderRevenueDev-importable.legacy.********************.js 4.6 kB 0 B
dotcom-rendering/dist/readerRevenueDevUtils.********************.js 3.87 kB 0 B
dotcom-rendering/dist/readerRevenueDevUtils.js 2.33 kB 0 B
dotcom-rendering/dist/readerRevenueDevUtils.legacy.********************.js 4.93 kB 0 B
dotcom-rendering/dist/ReaderRevenueLinks-importable.********************.js 5.87 kB 0 B
dotcom-rendering/dist/ReaderRevenueLinks-importable.legacy.********************.js 3.34 kB 0 B
dotcom-rendering/dist/relativeTime.********************.js 1.3 kB 0 B
dotcom-rendering/dist/relativeTime.legacy.********************.js 1.35 kB 0 B
dotcom-rendering/dist/RichLinkComponent-importable.********************.js 5.74 kB 0 B
dotcom-rendering/dist/RichLinkComponent-importable.legacy.********************.js 5.86 kB 0 B
dotcom-rendering/dist/SecureSignupIframe-importable.********************.js 8.43 kB 0 B
dotcom-rendering/dist/SecureSignupIframe-importable.legacy.********************.js 8.76 kB 0 B
dotcom-rendering/dist/sentry.********************.js 684 B 0 B
dotcom-rendering/dist/sentry.legacy.********************.js 691 B 0 B
dotcom-rendering/dist/sentryLoader.********************.js 10.2 kB 0 B
dotcom-rendering/dist/sentryLoader.legacy.********************.js 14.1 kB 0 B
dotcom-rendering/dist/SetABTests-importable.********************.js 8.23 kB 0 B
dotcom-rendering/dist/SetABTests-importable.legacy.********************.js 2.74 kB 0 B
dotcom-rendering/dist/ShareCount-importable.********************.js 3.59 kB 0 B
dotcom-rendering/dist/ShareCount-importable.legacy.********************.js 3.7 kB 0 B
dotcom-rendering/dist/shimport.********************.js 2.78 kB 0 B
dotcom-rendering/dist/shimport.legacy.********************.js 2.79 kB 0 B
dotcom-rendering/dist/ShowHideContainers-importable.********************.js 726 B 0 B
dotcom-rendering/dist/ShowHideContainers-importable.legacy.********************.js 761 B 0 B
dotcom-rendering/dist/SignInGateMain.********************.js 4.48 kB 0 B
dotcom-rendering/dist/SignInGateMain.js 2.87 kB 0 B
dotcom-rendering/dist/SignInGateMain.legacy.********************.js 4.56 kB 0 B
dotcom-rendering/dist/SignInGateSelector-importable.********************.js 5.2 kB 0 B
dotcom-rendering/dist/SignInGateSelector-importable.legacy.********************.js 5.49 kB 0 B
dotcom-rendering/dist/SlotBodyEnd-importable.********************.js 2.29 kB 0 B
dotcom-rendering/dist/SlotBodyEnd-importable.legacy.********************.js 2.93 kB 0 B
dotcom-rendering/dist/SpotifyBlockComponent-importable.********************.js 5.04 kB 0 B
dotcom-rendering/dist/SpotifyBlockComponent-importable.legacy.********************.js 5.26 kB 0 B
dotcom-rendering/dist/StickyBottomBanner-importable.********************.js 6.85 kB 0 B
dotcom-rendering/dist/StickyBottomBanner-importable.legacy.********************.js 7.84 kB 0 B
dotcom-rendering/dist/SubNav-importable.********************.js 3.26 kB 0 B
dotcom-rendering/dist/SubNav-importable.legacy.********************.js 3.38 kB 0 B
dotcom-rendering/dist/TimelineAtomWrapper-importable.********************.js 262 B 0 B
dotcom-rendering/dist/TimelineAtomWrapper-importable.legacy.********************.js 273 B 0 B
dotcom-rendering/dist/TopicFilterBank-importable.********************.js 3.73 kB 0 B
dotcom-rendering/dist/TopicFilterBank-importable.legacy.********************.js 3.84 kB 0 B
dotcom-rendering/dist/TopRightAdSlot-importable.********************.js 1.99 kB 0 B
dotcom-rendering/dist/TopRightAdSlot-importable.legacy.********************.js 2.02 kB 0 B
dotcom-rendering/dist/TweetBlockComponent-importable.********************.js 1.8 kB 0 B
dotcom-rendering/dist/TweetBlockComponent-importable.legacy.********************.js 1.79 kB 0 B
dotcom-rendering/dist/UnsafeEmbedBlockComponent-importable.********************.js 3.01 kB 0 B
dotcom-rendering/dist/UnsafeEmbedBlockComponent-importable.legacy.********************.js 3.11 kB 0 B
dotcom-rendering/dist/VideoFacebookBlockComponent-importable.********************.js 5.13 kB 0 B
dotcom-rendering/dist/VideoFacebookBlockComponent-importable.legacy.********************.js 5.35 kB 0 B
dotcom-rendering/dist/VineBlockComponent-importable.********************.js 2.81 kB 0 B
dotcom-rendering/dist/VineBlockComponent-importable.legacy.********************.js 2.91 kB 0 B
dotcom-rendering/dist/YoutubeBlockComponent-importable.********************.js 5.7 kB 0 B
dotcom-rendering/dist/YoutubeBlockComponent-importable.legacy.********************.js 6 kB 0 B

compressed-size-action

@github-actions
Copy link

github-actions bot commented Aug 25, 2022

⚡️ Lighthouse report for the changes in this PR

Lighthouse tested 2 URLs

⚠️ Budget exceeded for 1 of 10 audits.

Report for Article

tested url https://www.theguardian.com/commentisfree/2020/feb/08/hungary-now-for-the-new-right-what-venezuela-once-was-for-the-left

Category Status Expected Actual
First Contentful Paint 1500 1181
Largest Contentful Paint 3000 1902
Time to Interactive 3500 1646
Cumulative Layout Shift ⚠️ 0.002 0.004428
accessibility 0.97 1.000000

Report for Front

tested url https://www.theguardian.com/uk

Category Status Expected Actual
First Contentful Paint 1500 1206
Largest Contentful Paint 3000 2614
Time to Interactive 3500 2690
Cumulative Layout Shift 0.002 0.000000
accessibility 0.97 0.970000

Copy link
Contributor

@OllysCoding OllysCoding left a comment

Choose a reason for hiding this comment

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

Looks good!

@mxdvl
Copy link
Contributor Author

mxdvl commented Aug 25, 2022

I guess there are a few non-equivalent ways of checking whether an object has a property/method in js, right? I'm never clear on exactly which to use when. in looks to me like it should work here, but I'd be interested to know if you've got any guidelines on when to use what test?

Thanks for the question. The fact that you raised this means we might have to capture this information more clearly. I will merge this PR now we can use issue #5853 to discuss this further.

The reason I am using the in keyword here is because we have a linting rule against checking optional chaining on defined members of an object. As this method is defined in lib.dom.d.ts, we need to circumvent this rule.

window.performance.getEntriesByType?.('measure');
// Via https://typescript-eslint.io/rules/no-unnecessary-condition/ ESLint auto-fixes to:
window.performance.getEntriesByType('measure');

An alternative would be to disable that rule locally, which might be more explicit? What do you think, @bryophyta:

// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- not all browsers
window.performance.getEntriesByType?.('measure');

@mxdvl mxdvl merged commit 6d2e355 into main Aug 25, 2022
@mxdvl mxdvl deleted the mxdvl/safe-get-entries-by-type branch August 25, 2022 10:21
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.

3 participants