fix: bug when showing multiple ads on screen - FE-1623

.npmrc

@@ -0,0 +1,2 @@
+@artnetworldwide:registry=https://npm.pkg.github.com/
+always-auth=true

js/adslot.js → js/adslot.jsx

@@ -1,34 +1,47 @@
 import React from 'react';
-import PropTypes from 'prop-types';
 import DFPManager from './manager';
 import { Context } from './dfpslotsprovider';
 
 let dynamicAdCount = 0;
 
+/**
+ * @typedef {number[] | string} AdSizeItem
+ */
+
+/**
+ * @typedef {Object} SizeMappingEntry
+ * @property {[number, number]} viewport
+ * @property {Array<number[] | string>} sizes
+ */
+
+/**
+ * @typedef {Object} AdSlotProps
+ * @property {string} [dfpNetworkId]
+ * @property {string} [adUnit]
+ * @property {AdSizeItem[]} [sizes]
+ * @property {boolean} [renderOutOfThePage]
+ * @property {SizeMappingEntry[]} [sizeMapping]
+ * @property {boolean} [fetchNow]
+ * @property {Object} [adSenseAttributes]
+ * @property {Object} [targetingArguments]
+ * @property {(params: object) => void} [onSlotRender]
+ * @property {(params: object) => void} [onSlotRegister]
+ * @property {(params: object) => void} [onSlotIsViewable]
+ * @property {(params: object) => void} [onSlotVisibilityChanged]
+ * @property {(ctx: object) => boolean} [shouldRefresh]
+ * @property {string} [slotId]
+ * @property {string} [className]
+ */
+
+/**
+ * @typedef {AdSlotProps & { slotId?: string | null }} AdSlotState
+ */
+
+/**
+ * Google DFP / GPT ad slot. Must be rendered under {@link DFPSlotsProvider}.
+ * @extends {React.Component<AdSlotProps, AdSlotState>}
+ */
 export class AdSlot extends React.Component {
-  static propTypes = {
-    dfpNetworkId: PropTypes.string,
-    adUnit: PropTypes.string,
-    sizes: PropTypes.arrayOf(
-      PropTypes.oneOfType([
-        PropTypes.arrayOf(PropTypes.number),
-        PropTypes.string,
-      ]),
-    ),
-    renderOutOfThePage: PropTypes.bool,
-    sizeMapping: PropTypes.arrayOf(PropTypes.object),
-    fetchNow: PropTypes.bool,
-    adSenseAttributes: PropTypes.object,
-    targetingArguments: PropTypes.object,
-    onSlotRender: PropTypes.func,
-    onSlotRegister: PropTypes.func,
-    onSlotIsViewable: PropTypes.func,
-    onSlotVisibilityChanged: PropTypes.func,
-    shouldRefresh: PropTypes.func,
-    slotId: PropTypes.string,
-    className: PropTypes.string,
-  };
-
   static defaultProps = {
     fetchNow: false,
   };
@@ -49,20 +62,29 @@ export class AdSlot extends React.Component {
       slotId: this.props.slotId || null,
       className: this.props.className || '',
     };
-    this.adElementRef = React.createRef ? React.createRef() : (element) => {
-      this.adElementRef = element;
-    };
+    this.adElementRef = React.createRef
+      ? React.createRef()
+      : (element) => {
+          this.adElementRef = element;
+        };
+    /** Set in {@link #doRegisterSlot} when we notify the provider; cleared before {@link #releaseSlotCallback}. */
+    this._dfpNotifiedProvider = false;
   }
 
   componentDidMount() {
-    // register this ad-unit in the <DFPSlotProvider>, when available.
-    if (this.context !== undefined && this.context.newSlotCallback) {
-      this.context.newSlotCallback();
-    }
     this.registerSlot();
   }
 
   componentWillUnmount() {
+    const ctx = this.context;
+    if (
+      this._dfpNotifiedProvider &&
+      ctx !== undefined &&
+      typeof ctx.releaseSlotCallback === 'function'
+    ) {
+      ctx.releaseSlotCallback();
+      this._dfpNotifiedProvider = false;
+    }
     this.unregisterSlot();
   }
 
@@ -100,6 +122,14 @@ export class AdSlot extends React.Component {
   }
 
   doRegisterSlot() {
+    // Count this slot with the provider in the same phase as GPT registration (after slotId exists).
+    // Doing this in componentDidMount before setState caused totalSlots to get ahead of
+    // registeredSlots; React Strict Mode made that permanent so load() never ran.
+    const ctx = this.context;
+    if (ctx !== undefined && typeof ctx.newSlotCallback === 'function') {
+      ctx.newSlotCallback();
+      this._dfpNotifiedProvider = true;
+    }
     DFPManager.registerSlot({
       ...this.mapContextToAdSlotProps(),
       ...this.props,
@@ -118,9 +148,12 @@ export class AdSlot extends React.Component {
 
   registerSlot() {
     if (this.state.slotId === null) {
-      this.setState({
-        slotId: this.generateSlotId(),
-      }, this.doRegisterSlot);
+      this.setState(
+        {
+          slotId: this.generateSlotId(),
+        },
+        this.doRegisterSlot,
+      );
     } else {
       this.doRegisterSlot();
     }
@@ -204,18 +237,8 @@ export class AdSlot extends React.Component {
   }
 }
 
-if (Context === null) {
-  // React < 16.3
-  AdSlot.contextTypes = {
-    dfpNetworkId: PropTypes.string,
-    dfpAdUnit: PropTypes.string,
-    dfpSizeMapping: PropTypes.arrayOf(PropTypes.object),
-    dfpTargetingArguments: PropTypes.object,
-    newSlotCallback: PropTypes.func,
-  };
-} else {
+if (Context != null) {
   AdSlot.contextType = Context;
 }
 
-
 export default AdSlot;

js/dfpslotsprovider.js → js/dfpslotsprovider.jsx

@@ -1,61 +1,68 @@
 import React from 'react';
-import PropTypes from 'prop-types';
 import DFPManager from './manager';
 
 // React.createContext is undefined for React < 16.3
-export const Context = React.createContext ? React.createContext({
-  dfpNetworkId: null,
-  dfpAdUnit: null,
-  dfpSizeMapping: null,
-  dfpTargetingArguments: null,
-  newSlotCallback: null,
-}) : null;
+export const Context = React.createContext
+  ? React.createContext({
+      dfpNetworkId: null,
+      dfpAdUnit: null,
+      dfpSizeMapping: null,
+      dfpTargetingArguments: null,
+      newSlotCallback: null,
+      releaseSlotCallback: null,
+    })
+  : null;
 
-export default class DFPSlotsProvider extends React.Component {
-  static propTypes = {
-    children: PropTypes.oneOfType([
-      PropTypes.element,
-      PropTypes.array,
-    ]).isRequired,
-    autoLoad: PropTypes.bool,
-    autoReload: PropTypes.shape({
-      dfpNetworkId: PropTypes.bool,
-      personalizedAds: PropTypes.bool,
-      cookieOption: PropTypes.bool,
-      singleRequest: PropTypes.bool,
-      disableInitialLoad: PropTypes.bool,
-      adUnit: PropTypes.bool,
-      sizeMapping: PropTypes.bool,
-      adSenseAttributes: PropTypes.bool,
-      targetingArguments: PropTypes.bool,
-      collapseEmptyDivs: PropTypes.bool,
-      lazyLoad: PropTypes.bool,
-    }),
-    dfpNetworkId: PropTypes.string.isRequired,
-    personalizedAds: PropTypes.bool,
-    cookieOption: PropTypes.bool,
-    singleRequest: PropTypes.bool,
-    disableInitialLoad: PropTypes.bool,
-    adUnit: PropTypes.string,
-    sizeMapping: PropTypes.arrayOf(PropTypes.object),
-    adSenseAttributes: PropTypes.object,
-    targetingArguments: PropTypes.object,
-    collapseEmptyDivs: PropTypes.oneOfType([
-      PropTypes.bool,
-      PropTypes.object,
-    ]),
-    adSenseAttrs: PropTypes.object,
-    lazyLoad: PropTypes.oneOfType([
-      PropTypes.bool,
-      PropTypes.shape({
-        fetchMarginPercent: PropTypes.number,
-        renderMarginPercent: PropTypes.number,
-        mobileScaling: PropTypes.number,
-      }),
-    ]),
-    limitedAds: PropTypes.bool,
-  };
+/**
+ * @typedef {Object} AutoReloadConfig
+ * @property {boolean} [dfpNetworkId]
+ * @property {boolean} [personalizedAds]
+ * @property {boolean} [cookieOption]
+ * @property {boolean} [singleRequest]
+ * @property {boolean} [disableInitialLoad]
+ * @property {boolean} [adUnit]
+ * @property {boolean} [sizeMapping]
+ * @property {boolean} [adSenseAttributes]
+ * @property {boolean} [targetingArguments]
+ * @property {boolean} [collapseEmptyDivs]
+ * @property {boolean} [lazyLoad]
+ */
+
+/**
+ * @typedef {Object} LazyLoadConfig
+ * @property {number} [fetchMarginPercent]
+ * @property {number} [renderMarginPercent]
+ * @property {number} [mobileScaling]
+ */
+
+/**
+ * @typedef {import('react').ReactNode} ReactNode
+ */
 
+/**
+ * @typedef {Object} DFPSlotsProviderProps
+ * @property {ReactNode} children
+ * @property {boolean} [autoLoad]
+ * @property {AutoReloadConfig} [autoReload]
+ * @property {string} dfpNetworkId
+ * @property {boolean} [personalizedAds]
+ * @property {boolean} [cookieOption]
+ * @property {boolean} [singleRequest]
+ * @property {boolean} [disableInitialLoad]
+ * @property {string} [adUnit]
+ * @property {object[]} [sizeMapping]
+ * @property {Object} [adSenseAttributes]
+ * @property {Object} [targetingArguments]
+ * @property {boolean} [collapseEmptyDivs]
+ * @property {Object} [adSenseAttrs]
+ * @property {boolean | LazyLoadConfig} [lazyLoad]
+ * @property {boolean} [limitedAds]
+ */
+
+/**
+ * @extends {React.Component<DFPSlotsProviderProps>}
+ */
+export default class DFPSlotsProvider extends React.Component {
   static defaultProps = {
     autoLoad: true,
     autoReload: {
@@ -88,6 +95,7 @@ export default class DFPSlotsProvider extends React.Component {
     this.shouldReloadConfig = this.shouldReloadConfig.bind(this);
     this.attachLoadCallback = this.attachLoadCallback.bind(this);
     this.getContextValue = this.getContextValue.bind(this);
+    this.releaseSlotCallback = this.releaseSlotCallback.bind(this);
     this.loadAlreadyCalled = false;
     this.loadCallbackAttached = false;
     this.shouldReloadAds = false;
@@ -159,6 +167,7 @@ export default class DFPSlotsProvider extends React.Component {
         dfpSizeMapping,
         dfpTargetingArguments,
         newSlotCallback: this.newSlotCallback,
+        releaseSlotCallback: this.releaseSlotCallback,
       };
     }
     return this.contextValue;
@@ -193,12 +202,23 @@ export default class DFPSlotsProvider extends React.Component {
     this.totalSlots++;
   }
 
+  /** Pairs with {@link #newSlotCallback} when a slot unmounts (e.g. React Strict Mode). */
+  releaseSlotCallback() {
+    this.totalSlots = Math.max(0, this.totalSlots - 1);
+  }
+
   // Checks all the mounted children ads have been already registered
   // in the DFPManager before trying to call the gpt load scripts.
   // This is helpful when trying to fetch ads with a single request.
+  //
+  // Require totalSlots > 0 so we never fire load() when registeredSlots === 0
+  // and totalSlots === 0 (the old condition `>=` was true for 0 >= 0), which
+  // called enableServices() before any slot was defined and broke multi-slot
+  // pages.
   loadAdsIfPossible() {
     let r = false;
-    if (Object.keys(DFPManager.getRegisteredSlots()).length >= this.totalSlots) {
+    const registeredCount = Object.keys(DFPManager.getRegisteredSlots()).length;
+    if (this.totalSlots > 0 && registeredCount >= this.totalSlots) {
       DFPManager.removeListener('slotRegistered', this.loadAdsIfPossible);
       DFPManager.load();
       this.loadAlreadyCalled = true;
@@ -217,7 +237,10 @@ export default class DFPSlotsProvider extends React.Component {
         for (const i in attrs) {
           const propName = attrs[i];
           // eslint-disable-next-line
-          if (reloadConfig[propName] === true && this.props[propName] !== nextProps[propName]) {
+          if (
+            reloadConfig[propName] === true &&
+            this.props[propName] !== nextProps[propName]
+          ) {
             return true;
           }
         }
@@ -242,10 +265,11 @@ export default class DFPSlotsProvider extends React.Component {
 if (Context === null) {
   // React < 16.3
   DFPSlotsProvider.childContextTypes = {
-    dfpNetworkId: PropTypes.string,
-    dfpAdUnit: PropTypes.string,
-    dfpSizeMapping: PropTypes.arrayOf(PropTypes.object),
-    dfpTargetingArguments: PropTypes.object,
-    newSlotCallback: PropTypes.func,
+    dfpNetworkId: () => {},
+    dfpAdUnit: () => {},
+    dfpSizeMapping: () => {},
+    dfpTargetingArguments: () => {},
+    newSlotCallback: () => {},
+    releaseSlotCallback: () => {},
   };
 }