Rubicon project improvement/user sync

[mkendall07]

continued from here

[dbemiller]

I think this is my last pass... things are looking really good now.

[dbemiller]

String parsing is very fragile... would suggest one of the following instead:

• Change createTrackPixelIframeHtml so that it accepts a 'sandbox' option, and builds the right HTML from it
• Create the div object first, and then set the sandbox attribute programatically

[dbemiller]

This is unused/can be deleted.

[dbemiller]

This looks really good... but there's one small tweak I'd like to propose:

import { config } from 'src/config';

export function newUserSync(userSyncDependencies) { ... }

const cookiesAreSupported = !utils.isSafariBrowser() && utils.cookiesAreEnabled();
export const userSync = newUserSync({
  config: config.getConfig('userSync'),
  cookiesAreSupported: cookiesAreSupported
});

It solves a testability problem you described in userSync_spec (see the comment there), and also makes this module fit into the larger Prebid architecture described in #1510.

[dbemiller]

If you make the tweaks to userSync (see the comment in that file), this becomes testable very easily.

userSync = newUserSync({
  config: ...,
  cookiesAreSupported: false
})

[dbemiller]

I think this needs to be a stub?

Spies only observe things... so I think all the registerSync calls make real mutations to the real usersync global state.

Tests shouldn't have side effects like this.

[dbemiller]

If you're in a browser which doesn't support cookies, will the user syncs just build up inside the queue? Since this returns before calling queue = getDefaultQueue(). I'm not sure it's a big deal... but I suppose it could chew up memory if people run lots of auctions on the same page. Could probably wait for a secondary PR, though.

On that note... isn't this module basically a no-op if the browser doesn't support cookies? Again, perhaps a comment for a secondary PR, but... we could improve performance with something like:

export function newUserSync(userSyncDependencies) {
  if (!userSyncDependencies.cookiesAreSupported) {
    return {
      // Not pseudocode; these are literal no-ops
      registerSync: function() { }
      syncUsers: function() { }
      syncUsersOverride: function() { }
    }
  } else {
    // All the existing code & public API goes here
  }
}

[jaiminpanchal27]

A small improvement.
Since we have default value for userSync and there is not setter for userSync. It will always return an object so no need to keep a conditional check using ||
You have also kept a test case check to validate default config.

[harpere]

@jaiminpanchal27 userSync can be set with config.setConfig({userSync: null}); though I'm not sure why anyone would set it to null or another falsey value, unless maybe they thought it would turn it off.

[mkendall07]

@harpere
any updates here?

[mkendall07]

@harpere for now, let's drop these as we are only storing keys for S2S bidders and not client side. Later on we can integrate them, possibly by appending s2s${biddercode} to differentiate.