Adjust from defining all ads to only defining ads that have components

[Trav84]

Problem with Current Implementation:

The current implementation in Advertising.js defines all ads found in the config at once, regardless of whether they are actually needed on the current page. You can see this in the defineSlots() method (line 280), where the entire config is iterated over, and ads are defined for both GPT and services like Prebid or Amazon.

However, this approach assumes that all ads in the config should be rendered on the current page, which is problematic for sites with diverse ad layouts. Although the library allows for route-specific config files, this becomes unwieldy on larger sites with numerous layouts.

The Specific Issue:

For example, I manage ads on a site with dozens of unique page layouts, and I maintain a single unified config file. This leads to defining ads that may not exist on every page, resulting in errors like: "[GPT] Error in googletag.display: could not find div with id..."

These errors occur because googletag.display() is called for ads that have no corresponding DOM element. This not only clutters the console with warnings but can also impact performance, as unnecessary ads are processed.

My Current Workaround:

To avoid this, I use the plugin system to manually check if ad slots exist in the DOM before defining them. However, this approach has limitations. In some cases, the DOM elements for certain ads may not have been loaded by the time the plugin system checks, leading to ads being incorrectly skipped.

Recommended Solution:

Rather than defining all ads from the config upfront, I recommend deferring ad definition until activate() is called. This way, an ad would only be defined once its corresponding DOM element is present, ensuring that it is valid and necessary for the current page.

This would:

• Eliminate the need for route-specific config files by making a unified config more practical.
• Avoid errors caused by calling googletag.display() on ads without a corresponding DOM element.
• Reduce console spam and improve performance by ensuring that only the relevant ads are defined and displayed. This also makes debugging easier, as in the GPT console you'll only see actual ad definitions on the page, versus everything in your config file.

Next Steps:

Implementing this change would involve moving the ad definition call out of setupGPT() and into activate(). Also the display() call would need to be moved as well. Ads would only be defined when they are explicitly activated, meaning their DOM elements are already present and mounted. This would streamline ad management for sites with complex layouts and help reduce configuration complexity.

Alternatively, you could also still define all the ads, but only call the initial display() call (which registers the ad because disableInitialLoad has been called. However this still means defining ads that are not going to be displayed on the page, so it is not ideal.

[caffeinated-pixels]

Hi, thanks for the feedback.

This is an issue that we've also faced on Kijiji. Like you, our current workaround is to use route-specific configs. We've also been using dynamic configs that update based on the values of certain props. When you update the config this way, React Advertising tears down the previous setup and reinitializes GPT with the new config, but it's not ideal and can still result in GPT console warnings.

Your suggestion makes sense but it will need thoroughly testing. Hopefully, I can find time to test in an upcoming sprint.

[caffeinated-pixels]

I have a draft PR up (#142), still testing and fixing the unit tests.

[caffeinated-pixels]

@Trav84 Fixed the unit tests now, it would be great if you could take a look at the PR and test it in your own dev environment.

[Trav84]

I'll try and take a look next week!