CORE: allow to disable setting the pbjs global variable

[olafbuitelaar]

Type of change

• Bugfix

• Feature

• New bidder adapter

• Code style update (formatting, local variables)

• Refactoring (no functional changes, no api changes)

• Build related changes

• CI related changes

• Does this change affect user-facing APIs or examples documented on http://prebid.org?

• Other

Description of change

for embedding prebid as a pure module in other projects, it always has the side effect of settings the prebid global variable on the window object.
This PR, gives the user the option, to disable this behaviour, either by setting; defineGlobal to false in the package.json or via the options in the babel-loaded module.
This PR sets the prebid global everywhere to a localscope, and uses the function getGlobal to resolve the general reference.
when the defineGlobal is kept true, behaviour should be unaffected.

[dgirardi]

I am uneasy about this putting names in the module's scope without checking for conflicts, and the fact that - if I understand this right - this has the "side effect" of removing references to the global from module code; by that I mean that it's not clear from this code that that's the intent; it appears to be dealing with installedModules only. I think its better to be explicit.

What do you think about this strategy:

• refactor all uses of the $$PREBID_GLOBAL$$ macro, with the exception of prebidGlobal.js, to be simple and explicit uses of getGlobal(). const pbjs = getGlobal() seems to me much better than const $$PREBID_GLOBAL$$ = getGlobal() or just direct use of $$PREBID_GLOBAL$$ that are covertly translated to the first form through this black magic.
• remove the linter exception for $$PREBID_GLOBAL$$ to stop future circumvention of getGlobal(). Also, do not add one for $$USE_PREBID_GLOBAL$$, both can be single-file comment exceptions.
• for dealing with installedModules, I think this should try to stay invisible to the code's namespace. that means generating code like import {getGlobal as __r} from 'src/prebidGlobal.js; __r().installedModules.push(...), with some logic to use __r2 instead of __r if the latter is taken and so on (through scope.hasBinding, there's an example in here when dealing with FEATURES).

[olafbuitelaar]

• I've factored out all all references to $$PREBID_GLOBAL$$, except in the prebidGlobal.js. There was 1 module (rivrAnalyticsAdapter.js) which requires the global variable name.
• a common pattern i see, is that many modules just reference the global to get the version, e.g. $$PREBID_GLOBAL$$.version, maybe an utility function should be introduced for this, or maybe encourage the usage of 'v$prebid.version$'?
• i've removed the global eslint rules
• i've updated the babel plugin, to not introduce the extra variable, call the getGlobal() directly to set the modules, also i removed the different paths for using a global variable, or only use the internal instance.

[olafbuitelaar]

forget my comment about rivrAnalyticsAdapter.js, it uses the reference after all

[dgirardi]

maybe an utility function should be introduced for this, or maybe encourage the usage of 'v$prebid.version$'?

I like the first idea because it would work better with the minifier, but I don't think it has to be part of this PR. (I don't like using v$prebid.version$ because these macros are already more of a hack than I'd like).

The approach you have here is still in my opinion technically incorrect (the best kind of incorrect) because a module could theoretically use the name getGlobal for its own unrelated purposes, which makes the autogenerated import intrusive. It's not a huge deal but I think solving the problem also makes the code overall simpler: i pushed this update do demonstrate, which should also convince circleCI to run the tests.

[olafbuitelaar]

fair point, only right now it's possible to import getGlobal twice (once by the module, and once auto-generated), not sure how well this would finally get minimized. otherwise maybe the 2 solutions could get merged, where this new variable is only introduced in this if 1a3e72e#diff-d38260cead55420ead0e3bc806c91343b780d84c61577742edb484a4add1dc22L96 but i'm fine with it as it right now, as well.

[olafbuitelaar]

i would also prefer the utility function to resolve the version, but agree this shouldn't be part of this PR

[dgirardi]

I stole the same utility function idea for registerModule, so even if the minifier could not deal with the double import (I don't know either) it should not be an issue now. In fact there is an improvement as until now it could not touch the .installedModules property accessor, but it can rename the utility function.

[robertrmartinez]

I think we have the issue where the circle ci won't run since it is not the prebid org instance.

What exactly do we need to do to get it to run @dgirardi @ChrisHuie ?

[olafbuitelaar]

@dgirardi are you sure the src/prebidGlobal.js (1a3e72e#diff-d38260cead55420ead0e3bc806c91343b780d84c61577742edb484a4add1dc22R73) change will always work? As i'm having issues with it when using dynamic module importing e.g. import('prebid.js/modules/appnexusBidAdapter') and only the implementation (1a3e72e#diff-d38260cead55420ead0e3bc806c91343b780d84c61577742edb484a4add1dc22L96) with absolute relative path's (../) works for me in this case.

[olafbuitelaar]

@dgirardi it looks like your commit introduced a merge conflict again, since the rubicon adapter was updated, that change shouldn't be needed anymore, since that code is gone.

[dgirardi]

@olafbuitelaar thank you for calling out the relative path issue; it should be (finally) fixed.