Refactor commercial boot process #1764

emma-imber · 2025-01-24T15:06:29Z

What does this change?

Splits out the loadModules, recordCommercialMetrics and bootCommercial functions into a separate commercial-boot-utils file for tidiness. Also splits consented.ts into two files - ad-free.ts and consented-advertising.ts. This (in my opinion) makes the ad free logic easier to find and clearer to read. By doing this, we also get rid of the idea of "base" and "extra" modules, which were confusingly named.

Also removes the naming of the modules and no longer loads using the catchErrorsAndReport function. This also means we can delete the robust module as this was the only use of it.

Why?

Having explained this code several times over the last few weeks to different people, I have noticed a few parts that don't make immediate sense when you try to explain them (eg the naming of base vs extra modules). I think this little refactor is much more readable, and focuses the different bundle entry point files on the modules that they load.

Also, by having an ad-free specific file, we should be shipping less code to end users who don't see ads. I am also in favour of having the ad free check in the commercial file instead of the consented file, as I think it makes the purpose of each file much clearer to understand.

Would like to hear other people's thoughts on if this is an improvement or not!

changeset-bot · 2025-01-24T15:06:33Z

🦋 Changeset detected

Latest commit: d88d3ee

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@guardian/commercial	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2025-01-24T15:08:06Z

Deploy build 3073 of `dotcom::commercial-bundle` to CODE

All deployment options

From guardian/actions-riff-raff.

github-actions · 2025-01-24T15:09:22Z

Ad load time test results

For consented, top-above-nav took on average 4105ms to load.
For consentless, top-above-nav took on average 4169ms to load.

Test conditions:

5mbps download speed
1.5mbps upload speed
150ms latency

Jakeii · 2025-01-28T10:27:23Z

Looks much nicer to me!

Jakeii · 2025-01-28T10:29:30Z

The whole array of module names with init functions and catchErrorsAndReport looks to be just to help with error reporting?

I can't ever recall looking for errors in sentry using these, could we get rid and simplify further?

arelra · 2025-01-28T10:35:54Z

src/commercial.ts

+			/* webpackChunkName: "consentless-advertising" */
+			'./init/consentless-advertising'


This is very minor so feel free to ignore :) What do we gain anything by adding -advertising to all the chunks? Is it to help searching in dev tools?

It was only to make the distinction between code that loads an ad and the ad free code really explicit to anyone looking at it for the first time. I'm not set on it at all - happy to go with the majority opinion!

arelra · 2025-01-28T10:36:51Z

src/init/consented-advertising.ts

+
+// all modules needed for commercial code and ads to run
+const commercialModules: Modules = [
+	['cm-adFreeSlotRemoveFronts', adFreeSlotRemove],


While we're here... do we ned the cm- prefix to all the modules any longer. I suspect its something we carried over from Frontend. Could be a follow up.

Yeah good point - I think this ties in with Jake's point about reporting. If we're only using the module names for error reporting, maybe we can remove them altogether

arelra

Nice refactor ✨ !

emma-imber · 2025-01-28T10:41:22Z

The whole array of module names with init functions and catchErrorsAndReport looks to be just to help with error reporting?

I can't ever recall looking for errors in sentry using these, could we get rid and simplify further?

Yeah I don't think I've ever used this either! It's also the only bit of code using catchErrorsAndReport, so we could delete the robust file if we got rid

i-hardy · 2025-01-29T14:45:42Z

src/init/consented-advertising.ts

+	initDfpListeners,
+	// Permutive init code must run before google tag enableServices()
+	// The permutive lib however is loaded async with the third party tags
+	() => initPermutive().then(prepareGoogletag),


Maybe this could be part of a separate bit of work to tidy up the Permutive/Googletag code, but it would be nice if this could be a module unto itself like the others rather than a random hard-coded function and comment breaking up the array

I'm going to address this in a separate PR, but yes, agreed!

i-hardy · 2025-01-29T14:48:37Z

src/lib/init-utils.ts

+	}
+};
+
+export { bootCommercial };


I'm a bit allergic to everything being called 'init' in any case, but given the export maybe this file could be named something like 'commercial-boot-utils'? I think it would be a little clearer. Breaking these out from the main file is a huge improvement though!

i-hardy · 2025-01-29T14:52:14Z

src/commercial.ts

@@ -18,13 +18,18 @@ void (async () => {
 		!commercialFeatures.adFree


Could this condition be moved out into a named function that describes this particular state?

Eg.

Suggested change

!commercialFeatures.adFree

function shouldRunConsentless(consentState): boolean {

return window.guardian.config.switches.optOutAdvertising &&

consentState.tcfv2 &&

!getConsentFor('googletag', consentState) &&

!commercialFeatures.adFree;

}

...

if (shouldRunConsentless(consentState)) {

It would improve readability and make it clearer what all these conditions actually add up towards.

cemms1

This feels much easier to grok! 👏

prout-bot · 2025-02-03T14:28:12Z

Seen on PROD (merged by @emma-imber 1 minute and 42 seconds ago) Please check your changes!

Refactor module loading to make it easier to read

89534d2

emma-imber changed the title ~~Refactor module loading to make it easier to read~~ Refactor commercial boot process Jan 24, 2025

arelra reviewed Jan 28, 2025

View reviewed changes

arelra approved these changes Jan 28, 2025

View reviewed changes

emma-imber added 4 commits January 28, 2025 10:51

Move permutive comment to the right place

76c5536

Don't use catchErrorsAndReport to load modules

7bf3ed9

Remove duplicate modules loaded event

579c050

Fix module loading

abf1b4e

i-hardy reviewed Jan 29, 2025

View reviewed changes

emma-imber and others added 3 commits January 30, 2025 10:30

Rename init-utils as commercial-boot-utils

304bbe0

Separate consentless boot checks into named const for readability

cc15408

Merge branch 'main' into ei/refactor-boot-logic

9e0c5e1

emma-imber marked this pull request as ready for review January 30, 2025 11:30

emma-imber requested a review from a team as a code owner January 30, 2025 11:30

Changeset

77a618d

cemms1 approved these changes Feb 3, 2025

View reviewed changes

emma-imber added 2 commits February 3, 2025 10:34

Merge branch 'main' into ei/refactor-boot-logic

5c5a306

Merge branch 'main' into ei/refactor-boot-logic

d88d3ee

emma-imber merged commit 84e8bfb into main Feb 3, 2025
14 checks passed

emma-imber deleted the ei/refactor-boot-logic branch February 3, 2025 14:26

prout-bot added the Pending-on-PROD label Feb 3, 2025

prout-bot added Seen-on-PROD and removed Pending-on-PROD labels Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor commercial boot process #1764

Refactor commercial boot process #1764

emma-imber commented Jan 24, 2025 •

edited

Loading

changeset-bot bot commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

Jakeii commented Jan 28, 2025

Jakeii commented Jan 28, 2025

arelra Jan 28, 2025

emma-imber Jan 28, 2025

arelra Jan 28, 2025

emma-imber Jan 28, 2025

arelra left a comment

emma-imber commented Jan 28, 2025

i-hardy Jan 29, 2025

emma-imber Jan 30, 2025

i-hardy Jan 29, 2025

i-hardy Jan 29, 2025 •

edited

Loading

cemms1 left a comment

prout-bot commented Feb 3, 2025

		/* webpackChunkName: "consentless-advertising" */
		'./init/consentless-advertising'

		@@ -18,13 +18,18 @@ void (async () => {
		!commercialFeatures.adFree

-		!commercialFeatures.adFree
+		function shouldRunConsentless(consentState): boolean {
+				return window.guardian.config.switches.optOutAdvertising &&
+				consentState.tcfv2 &&
+				!getConsentFor('googletag', consentState) &&
+				!commercialFeatures.adFree;
+		}
+		...
+		if (shouldRunConsentless(consentState)) {

Refactor commercial boot process #1764

Refactor commercial boot process #1764

Conversation

emma-imber commented Jan 24, 2025 • edited Loading

What does this change?

Why?

changeset-bot bot commented Jan 24, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Jan 24, 2025 • edited Loading

github-actions bot commented Jan 24, 2025 • edited Loading

Ad load time test results

Jakeii commented Jan 28, 2025

Jakeii commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arelra left a comment

Choose a reason for hiding this comment

emma-imber commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-hardy Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

cemms1 left a comment

Choose a reason for hiding this comment

prout-bot commented Feb 3, 2025

emma-imber commented Jan 24, 2025 •

edited

Loading

changeset-bot bot commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

i-hardy Jan 29, 2025 •

edited

Loading