Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORE: allow to disable setting the pbjs global variable #9568

Merged
merged 12 commits into from
Mar 29, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ module.exports = {
],
globals: {
'$$PREBID_GLOBAL$$': false,
'$$DEFINE_PREBID_GLOBAL$$': false,
'BROWSERSTACK_USERNAME': false,
'BROWSERSTACK_KEY': false,
'FEATURES': 'readonly',
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"prebid"
],
"globalVarName": "pbjs",
"defineGlobal": true,
"author": "the prebid.js contributors",
"license": "Apache-2.0",
"engines": {
Expand Down
29 changes: 26 additions & 3 deletions plugins/pbjsGlobals.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@ function getNpmVersion(version) {

module.exports = function(api, options) {
const pbGlobal = options.globalVarName || prebid.globalVarName;
const defineGlobal = typeof (options.defineGlobal) !== 'undefined' ? options.defineGlobal : prebid.defineGlobal;
const features = featureMap(options.disableFeatures);
let replace = {
'$prebid.version$': prebid.version,
'$$PREBID_GLOBAL$$': pbGlobal,
'$$DEFINE_PREBID_GLOBAL$$': defineGlobal,
'$$REPO_AND_VERSION$$': `${prebid.repository.url.split('/')[3]}_prebid_${prebid.version}`,
'$$PREBID_DIST_URL_BASE$$': options.prebidDistUrlBase || `https://cdn.jsdelivr.net/npm/prebid.js@${getNpmVersion(prebid.version)}/dist/`
};
Expand All @@ -58,21 +60,42 @@ module.exports = function(api, options) {
return null;
}

function hasGetGlobalsImport(body) {
for (let i = 0; i < body.length; i++) {
if (body[i].type === 'ImportDeclaration' && body[i].source.value.match(/prebidGlobal(\.js)?$/)) {
for (let j = 0; j < body[i].specifiers.length; j++) {
if (body[i].specifiers[j].local.name === 'getGlobal') {
return true;
}
}
}
}
return false;
}

return {
visitor: {
Program(path, state) {
const modName = getModuleName(state.filename);
if (modName != null) {
// append "registration" of module file to $$PREBID_GLOBAL$$.installedModules
path.node.body.push(...api.parse(`window.${pbGlobal}.installedModules.push('${modName}');`, {filename: state.filename}).program.body);

if (defineGlobal) {
path.node.body.push(...api.parse(`window.${pbGlobal}.installedModules.push('${modName}');`, {filename: state.filename}).program.body);
} else {
if (!hasGetGlobalsImport(path.node.body)) {
path.node.body.unshift(...api.parse(`import {getGlobal} from '../src/prebidGlobal.js';const ${pbGlobal} = getGlobal();`, {filename: state.filename}).program.body);
Copy link
Collaborator

@dgirardi dgirardi Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@olafbuitelaar olafbuitelaar Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@olafbuitelaar olafbuitelaar Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
path.node.body.push(...api.parse(`${pbGlobal}.installedModules.push('${modName}');`, {filename: state.filename}).program.body);
}
}
},
StringLiteral(path) {
Object.keys(replace).forEach(name => {
if (path.node.value.includes(name)) {
path.node.value = path.node.value.replace(
new RegExp(escapeRegExp(name), 'g'),
replace[name]
replace[name].toString()
);
}
});
Expand Down Expand Up @@ -102,7 +125,7 @@ module.exports = function(api, options) {
);
} else {
path.replaceWith(
t.Identifier(replace[name])
t.Identifier(replace[name].toString())
);
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/Renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ import {
logError, logWarn, logMessage, deepAccess
} from './utils.js';
import {find} from './polyfill.js';
import {getGlobal} from './prebidGlobal.js';

const $$PREBID_GLOBAL$$ = getGlobal();
const moduleCode = 'outstream';

/**
Expand Down
3 changes: 3 additions & 0 deletions src/auction.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ import {GreedyPromise} from './utils/promise.js';
import {useMetrics} from './utils/perfMetrics.js';
import {createBid} from './bidfactory.js';
import {adjustCpm} from './utils/cpm.js';
import {getGlobal} from './prebidGlobal.js';

const { syncUsers } = userSync;

Expand All @@ -111,6 +112,8 @@ const outstandingRequests = {};
const sourceInfo = {};
const queuedCalls = [];

const $$PREBID_GLOBAL$$ = getGlobal();

/**
* Clear global state for tests
*/
Expand Down
15 changes: 9 additions & 6 deletions src/prebidGlobal.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
// if $$PREBID_GLOBAL$$ already exists in global document scope, use it, if not, create the object
// global defination should happen BEFORE imports to avoid global undefined errors.
window.$$PREBID_GLOBAL$$ = (window.$$PREBID_GLOBAL$$ || {});
window.$$PREBID_GLOBAL$$.cmd = window.$$PREBID_GLOBAL$$.cmd || [];
window.$$PREBID_GLOBAL$$.que = window.$$PREBID_GLOBAL$$.que || [];
const scope = !$$DEFINE_PREBID_GLOBAL$$ ? {} : window;
scope.$$PREBID_GLOBAL$$ = (scope.$$PREBID_GLOBAL$$ || {});
scope.$$PREBID_GLOBAL$$.cmd = scope.$$PREBID_GLOBAL$$.cmd || [];
scope.$$PREBID_GLOBAL$$.que = scope.$$PREBID_GLOBAL$$.que || [];

// create a pbjs global pointer
window._pbjsGlobals = window._pbjsGlobals || [];
window._pbjsGlobals.push('$$PREBID_GLOBAL$$');
if (scope === window) {
scope._pbjsGlobals = scope._pbjsGlobals || [];
scope._pbjsGlobals.push('$$PREBID_GLOBAL$$');
}

export function getGlobal() {
return window.$$PREBID_GLOBAL$$;
return scope.$$PREBID_GLOBAL$$;
}
3 changes: 3 additions & 0 deletions src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import clone from 'just-clone';
import {find, includes} from './polyfill.js';
import CONSTANTS from './constants.json';
import {GreedyPromise} from './utils/promise.js';
import {getGlobal} from './prebidGlobal.js';
export { default as deepAccess } from 'dlv/index.js';
export { dset as deepSetValue } from 'dset';

Expand All @@ -21,6 +22,8 @@ let consoleErrorExists = Boolean(consoleExists && window.console.error);

let eventEmitter;

const $$PREBID_GLOBAL$$ = getGlobal();

export function _setEventEmitter(emitFn) {
// called from events.js - this hoop is to avoid circular imports
eventEmitter = emitFn;
Expand Down