CommercialMetrics includes ad blocker use

[zekehuntergreen]

What does this change?

• Upgrade to newest version of commercial-core, which includes a change to sendCommercialMetrics so that it accepts an optional parameter adBlockerInUse
• Pass the new parameter to sendCommercialMetrics

Why?

We'd like to collect data on our readers' use of ad blockers

frontend → guardian/frontend#24191

[github-actions]

Size Change: +11 B (0%)

Total Size: 949 kB

Filename	Size	Change
dist/react.js	147 kB	+6 B (0%)
dist/react.legacy.js	153 kB	+5 B (0%)

ℹ️ View Unchanged

Filename	Size
dist/922.js	3.59 kB
dist/922.legacy.js	3.69 kB
dist/974.js	21.1 kB
dist/974.legacy.js	21.1 kB
dist/atomIframe.js	1.87 kB
dist/atomIframe.legacy.js	2.13 kB
dist/braze-web-sdk-core.js	36.1 kB
dist/braze-web-sdk-core.legacy.js	36.1 kB
dist/coreVitals.js	3.73 kB
dist/coreVitals.legacy.js	6.43 kB
dist/dynamicImport.js	3 kB
dist/dynamicImport.legacy.js	3.27 kB
dist/EditionDropdown.js	695 B
dist/EditionDropdown.legacy.js	704 B
dist/elements-CalloutBlockComponent.js	5.85 kB
dist/elements-CalloutBlockComponent.legacy.js	6.2 kB
dist/elements-DocumentBlockComponent.js	574 B
dist/elements-DocumentBlockComponent.legacy.js	604 B
dist/elements-InstagramBlockComponent.js	435 B
dist/elements-InstagramBlockComponent.legacy.js	453 B
dist/elements-InteractiveBlockComponent.js	2.58 kB
dist/elements-InteractiveBlockComponent.legacy.js	2.68 kB
dist/elements-InteractiveContentsBlockElement.js	1.9 kB
dist/elements-InteractiveContentsBlockElement.legacy.js	1.98 kB
dist/elements-MapEmbedBlockComponent.js	1.54 kB
dist/elements-MapEmbedBlockComponent.legacy.js	1.6 kB
dist/elements-RichLinkComponent.js	3.3 kB
dist/elements-RichLinkComponent.legacy.js	3.34 kB
dist/elements-SpotifyBlockComponent.js	1.47 kB
dist/elements-SpotifyBlockComponent.legacy.js	1.53 kB
dist/elements-VideoFacebookBlockComponent.js	1.55 kB
dist/elements-VideoFacebookBlockComponent.legacy.js	1.6 kB
dist/elements-VineBlockComponent.js	586 B
dist/elements-VineBlockComponent.legacy.js	601 B
dist/elements-YoutubeBlockComponent.js	2.1 kB
dist/elements-YoutubeBlockComponent.legacy.js	2.18 kB
dist/embedIframe.js	1.88 kB
dist/embedIframe.legacy.js	2.13 kB
dist/frontend.server.js	299 kB
dist/ga.js	3.87 kB
dist/ga.legacy.js	4.1 kB
dist/GetMatchStats.js	3.29 kB
dist/GetMatchStats.legacy.js	3.36 kB
dist/guardian-braze-components-banner.js	9.85 kB
dist/guardian-braze-components-banner.legacy.js	9.86 kB
dist/guardian-braze-components-end-of-article.js	6.54 kB
dist/guardian-braze-components-end-of-article.legacy.js	6.54 kB
dist/MostViewedFooterData.js	6.35 kB
dist/MostViewedFooterData.legacy.js	6.45 kB
dist/MostViewedRightWrapper.js	5.65 kB
dist/MostViewedRightWrapper.legacy.js	5.9 kB
dist/newsletterEmbedIframe.js	1.83 kB
dist/newsletterEmbedIframe.legacy.js	2.08 kB
dist/OnwardsLower.js	11.4 kB
dist/OnwardsLower.legacy.js	11.7 kB
dist/OnwardsUpper.js	12.5 kB
dist/OnwardsUpper.legacy.js	12.7 kB
dist/ophan.js	7.22 kB
dist/ophan.legacy.js	7.41 kB
dist/sentry.js	675 B
dist/sentry.legacy.js	686 B
dist/sentryLoader.js	3.25 kB
dist/sentryLoader.legacy.js	5.94 kB
dist/shimport.js	2.75 kB
dist/shimport.legacy.js	2.76 kB
dist/SignInGateMain.js	1.86 kB
dist/SignInGateMain.legacy.js	1.9 kB

_{compressed-size-action}

[oliverlloyd]

I think it would be better to replace this promise with useAdBlockInUse.

Having async calls which set state at the root of react components is likely to cause race conditions because you're fighting with React's re-rendering lifecycle rather than using it

[zekehuntergreen]

Thanks for the feedback, @oliverlloyd !
With @mxdvl we discussed using useAdBlockInUse but agreed that we can't use the function as it is now since it doesn't set the state of isInUse to false. In order to use useAdBlockInUse I think we'd need to change this line to setIsInUse(blockerDetected). The comment just above that line suggested that we rely on the current behavior but looking at its usage, I think we might be able to make the change without breaking anything.
An alternative might be to add a parameter like onlySetTrue (or something) to useAdBlockInUse so that we could toggle between the current behavior (only set the state of isInUse if it's true) and new behavior (always set the value). This seemed less clean to me than a direct call to isAdBlockInUse in CommercialMetrics.

[oliverlloyd]

doesn't set the state of isInUse to false

Ah, I see. Yes, that's true, it doesn't handle the use case where an action is required when no ad blocker is in use

I think we might be able to make the change without breaking anything

I agree. It currently only changes state in the positive case purely as a micro optimisation based on the assumption we would only ever want to use this logic to decide to do things when an ad blocker is detected. This is illustraighted with the WhenAdBlockInUse component. But it seems that assumption has been proven wrong!

[oliverlloyd]

Scratch my comment about needing useOnce. @zekehuntergreen rightly pointed out the sentCommercialMetrics state prevents double submission so the use of useOnce is only needed for the nice here, not as an absolute.