ADR: Propose commercial bundle refactor

bundle/src/init/ad-free.ts

@@ -5,7 +5,7 @@ import { init as initIpsosMori } from './consented/ipsos-mori';
 import { removeDisabledSlots as closeDisabledSlots } from './consented/remove-slots';
 import { initTeadsCookieless } from './consented/teads-cookieless';
 import { init as initTrackGpcSignal } from './consented/track-gpc-signal';
-import { init as initTrackScrollDepth } from './consented/track-scroll-depth';
+import { initTrackScrollDepth } from './consented/track-scroll-depth';
 import { init as initPages } from './pages';
 
 // modules not related to ad loading

bundle/src/init/consented-advertising.ts

@@ -17,7 +17,7 @@ import { removeDisabledSlots as closeDisabledSlots } from './consented/remove-sl
 import { initTeadsCookieless } from './consented/teads-cookieless';
 import { init as initThirdPartyTags } from './consented/third-party-tags';
 import { init as initTrackGpcSignal } from './consented/track-gpc-signal';
-import { init as initTrackScrollDepth } from './consented/track-scroll-depth';
+import { initTrackScrollDepth } from './consented/track-scroll-depth';
 import { init as initPages } from './pages';
 import { reloadPageOnConsentChange } from './shared/reload-page-on-consent-change';
 import { init as setAdTestCookie } from './shared/set-adtest-cookie';

bundle/src/init/consented/ad-free-slot-remove.ts

@@ -5,6 +5,8 @@ import { removeSlots } from './remove-slots';
 /**
  * If the user is ad-free, remove all ad slots on the page,
  * as the ad slots are still left on the page on fronts, and mostpop on articles
+ *
+ * @todo - this is included in the consented flow too. Why?
  */
 const adFreeSlotRemove = once(() => {
 	if (!commercialFeatures.adFree) {

bundle/src/lib/track-scroll-depth.spec.ts → bundle/src/init/consented/track-scroll-depth.spec.ts

@@ -1,7 +1,9 @@
 import { EventTimer } from '@guardian/commercial-core/event-timer';
-import { initTrackScrollDepth } from './track-scroll-depth';
+import { _ } from './track-scroll-depth';
 
-describe('initTrackScrollDepth', () => {
+const { trackScrollDepth } = _;
+
+describe('trackScrollDepth', () => {
 	const observe = jest.fn();
 	class IntersectionObserver {
 		constructor() {
@@ -46,7 +48,7 @@ describe('initTrackScrollDepth', () => {
 		const eventTimer = EventTimer.get();
 		window.innerHeight = 100;
 
-		initTrackScrollDepth();
+		trackScrollDepth();
 
 		// height of viewport recorded
 		expect(eventTimer.properties['pageHeightVH']).toEqual(10);

bundle/src/init/consented/track-scroll-depth.ts

@@ -1,18 +1,73 @@
+import { EventTimer } from '@guardian/commercial-core/event-timer';
 import { log, onConsent } from '@guardian/libs';
-import { initTrackScrollDepth } from '../../lib/track-scroll-depth';
+
+/**
+ * Collect commercial metrics on scroll depth
+ * Insert hidden elements at intervals of 1 viewport height
+ * then use an intersection observer to mark the time when the viewport intersects with these elements.
+ * Approach inspired by https://gist.github.com/bgreater/2412517f5a3f9c6fc4cafeb1ca71384f
+ */
+const trackScrollDepth = () => {
+	const pageHeight = document.body.offsetHeight;
+	const intViewportHeight = window.innerHeight;
+
+	// this if statement is here to handle a bug in Firefox in Android where the innerHeight
+	// of a new tab can be 0, so we end up dividing by 0 and looping through infinity
+	if (intViewportHeight === 0) {
+		log(
+			'commercial',
+			`Not tracking scroll depth because viewport height cannot be determined`,
+		);
+		return;
+	}
+
+	// how many viewports tall is the page?
+	const pageHeightVH = Math.floor(pageHeight / intViewportHeight);
+	const eventTimer = EventTimer.get();
+	eventTimer.setProperty('pageHeightVH', pageHeightVH);
+
+	const observer = new IntersectionObserver(
+		/* istanbul ignore next */
+		(entries) => {
+			entries.forEach((entry) => {
+				if (entry.isIntersecting) {
+					const currentDepthVH = String(
+						entry.target.getAttribute('data-depth'),
+					);
+					log('commercial', `current scroll depth ${currentDepthVH}`);
+					eventTimer.mark(`scroll-depth-vh-${currentDepthVH}`);
+					observer.unobserve(entry.target);
+				}
+			});
+		},
+	);
+
+	for (let depth = 1; depth <= pageHeightVH; depth++) {
+		const div = document.createElement('div');
+		div.dataset.depth = String(depth);
+		div.style.top = String(100 * depth) + '%';
+		div.style.position = 'absolute';
+		div.className = 'scroll-depth-marker';
+		document.body.appendChild(div);
+		observer.observe(div);
+	}
+};
+
+// Exports for testing only
+export const _ = { trackScrollDepth };
 
 /**
  * Initialise scroll depth / velocity tracking if user has consented to relevant purposes.
  * @returns Promise
  */
-export const init = async (): Promise<void> => {
+export const initTrackScrollDepth = async (): Promise<void> => {
 	const state = await onConsent();
 	if (
 		// Purpose 8 - Measure content performance
 		(state.framework == 'tcfv2' && !!state.tcfv2?.consents[8]) ||
 		state.canTarget
 	) {
-		initTrackScrollDepth();
+		trackScrollDepth();
 		log('commercial', 'tracking scroll depth');
 	} else {
 		log('commercial', 'No consent to track scroll depth');

bundle/src/lib/messenger/type.ts

@@ -7,6 +7,10 @@ const setType = (adSlotType: string, adSlot: Element) =>
 		adSlot.classList.add(`ad-slot--${adSlotType}`);
 	});
 
+/**
+ * This function has side effects via DOM mutations
+ * @param register a register listener
+ */
 const init = (register: RegisterListener): void => {
 	register('type', (specs, ret, iframe) => {
 		const adSlot = iframe?.closest('.js-ad-slot');

bundle/src/lib/construct-query.ts → bundle/src/lib/utils/construct-query.ts

@@ -1,4 +1,4 @@
-import type { MaybeArray } from './types';
+import type { MaybeArray } from '../types';
 
 const constructQuery = (
 	query: Record<string, MaybeArray<string | number | boolean>>,

bundle/src/services/README.md

@@ -0,0 +1,14 @@
+# Commercial Services
+
+This directory contains services which have interfaces we can interact with in a consistent and reliable way
+
+# Boot service
+
+TODO - implement this
+
+# Advert service
+
+TODO - implement this
+
+Question - should we separate this into "define" and "display"?
+

docs/architecture/proposals/004-bundle-refactoring.md

@@ -0,0 +1,91 @@
+# Refactor the bundle code
+
+## Status: Proposed
+
+## Context
+
+The commercial bundle code is very difficult to understand and follow.
+Making changes can feel dangerous and it's not always obvious where to put new files.
+
+We would like to refactor this to work in the most logical way possible to make it easier for both developers in the CommDev team to understand as well as contributors from across the department or even externally.
+
+## Proposal
+
+We propose the repo is structured in the following way:
+
+The top level boot for each of the three commercial processes (ad free, consented, consentless) should be minimal. It should only contain the essentials needed for understanding the process.
+
+```typescript
+await boot();
+await insertAds();
+await updateAds();
+```
+
+### Services
+
+A lot of the bundle code can be separated into distinct “services”. Each service has one main concern and should have a clearly defined interface. This means that any service could in theory be totally replaced by another one with the same type definition.
+
+We could separate the code into the following services:
+
+- **boot** (scripts needed to load the commercial bundle on the page, including tracking page targeting, loading third party scripts etc)
+- **define / slot** (defines ad slots for use in various other services)
+- **insert** (contains spacefinder and other dynamic ad slot insertion)
+- **ab / experiments** (contains ab tests or experiments)
+- **header bidding** (service for prebid/adx/a9 etc)
+- **messenger** (responsible for communication between iframes and the top layer)
+
+### Boot
+
+The consented-advertising file contains lots of information about init scripts that is hard to follow.
+
+We should audit the init scripts to understand what is needed at what stage and clearly annotate the code with priority order + dependencies
+
+This should include what is currently known as `commercialFeatures` and should change `commercialFeatures` from a class into a JSON object or similar.
+
+Open questions:
+
+- How do we know what the correct order/priority is for these init scripts?
+- What is comscore? Why does it need to run first? Why does it run on ad free?
+-
+
+### insertAds
+
+TODO
+
+### updateAds
+
+TODO
+
+### Directory Structure
+
+```sh
+│ index.ts # Entrypoint
+│   └──
+├──────
+
+```
+
+## Coding Standards
+
+We propose establishing the following coding standards:
+
+- **Avoid using the same names for internal functions and methods as external ones**.
+  Do not use the same command for a Google method as you would for a commercial one
+
+- **Avoid renaming files on import**. This improves understanding of the code
+  e.g.
+
+    ```typescript
+    // Bad
+    import { init as initGoogleTag } from '../googletag';
+
+    // Good
+    import { initGoogleTag } from '../googletag';
+    ```
+
+- **Use pure functions** and avoid using classes where possible. The commercial bundle does not really need to use classes
+  When side effects are necessary (like DOM manipulation), clearly note this in a JSDoc comment. Ideally we would include an eslint rule to prevent functions with side effects from being added without a suitable explanation for why this is happening
+
+- **Types** should live alongside their companion code in the same file where possible.
+    - If this isn't possible because the types are transient or global, they can live in the types directory
+- **Services** should have clearly defined interfaces.