Advanced Size Mapping - Code Refactoring.

[Fawke]

Type of change

• Bugfix
• Refactoring (no functional changes, no api changes)

Description of change

• checkAdUnitSetup in src/prebid.js has been refactored to not mutate the adUnit object. Similarly, helper functions such as validateBannerMediaType, validateVideoMediaType and validateNativeMediaType have been refactor to avoid mutation of the adUnit object.

• A similar function, checkAdUnitSetupHook in modules/sizeMappingV2.js is also refactored to avoid mutation of adUnit object.

• sizeConfig validation checks has been moved to a single function, validateSizeConfig which returns a boolean, and does not mutate the adUnit object.

• Re-write some of the comments in modules/sizeMappingV2.js to make them more descriptive.

• Re-write / shorten / remove some log messages in modules/sizeMappingV2.js

• A test has been skipped in pubCommonId_spec.js file. The test is failing, due to reasons not related to this change, as far as I know. I'll request someone from pubCommonId to take a look.

• Resolves Advanced Size Mapping Module bug empties config on refresh #5458. This bug was caused because we were mutating the adUnit object inside the checkAdUnitSetupHook function in modules/sizeMappingV2.js file.

[lgtm-com]

This pull request introduces 1 alert when merging d6004ba into a2ec9c2 - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

[Fawke]

Moving call to checkAdUnitSetup before we calculate the value of adUnitCodes. This is required because if some adUnit is removed in the function ,checkAdUnitSetup, then, the code below will incorrectly include the deactivated adUnit's code in the adUnitCodes variable.

[benjaminclot]

@Fawke why do you use "caching" (cacheHits)? Won't that cause a problem when somebody triggers an ad refresh after a window resize?

[Fawke]

@Fawke why do you use "caching" (cacheHits)? Won't that cause a problem when somebody triggers an ad refresh after a window resize?

@benjaminclot Caching is an internal implementation detail which should not effect page refreshes after a window resize. To give you an overview, caching stores a data structure which is mapped to an auctionId.

[{
  'auction-id-1': {
    adUnits: [
      <<contains details about adunits after size mapping checks>>
    ],
    cacheHits:1
  }
}]

This is done because we don't want to do sizeMapping processing for each bidder separately. It's done for the first bidder, stored in the cache, the next bidder picks up the details from this object and we increment the cacheHits.

If you refresh the page, it'll be a new auction (a new call to requestBids), which won't read data from the old auction, but create a new object. So, I think, it should be fine.

[benjaminclot]

@Fawke I was talking about the next call to pbjs.requestBids() but you're right, as long as the cache is linked to the auctionId, we'll be fine.

[bretg]

@Fawke - any docs updates?

[Fawke]

@bretg Not really, no changes to the API.

[bretg]

Cool - with there aren't changes to the interface, I'll remove myself from the reviewers.

[robertrmartinez]

looks good, just the question about the pubmatic skipped test.

[robertrmartinez]

👍 on the maps based off the mediaType, cool stuff

[robertrmartinez]

Why are we skipping this?

[Fawke]

If we don't skip this test, this fails!

What I could gather from this is that the test that I have skipped, it disables autocreate of bid.crumbs.pubcid. Then, later, this configuration is not cleared which is leading to the test case, I mentioned above, to fail!

If I find time later, will try to dig deep to ascertain the cause of the failure, but for now, everything seems to be working fine, we can merge this!