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.

Sign up for GitHub

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

Create module registry: update build to add installModules array to pbjs global #6601

Merged
merged 5 commits into from
Apr 22, 2021
Merged

Create module registry: update build to add installModules array to pbjs global #6601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad27af7
- Remove module list comment on build
mmoschovas Apr 15, 2021
5d45d11
Removed unused code
mmoschovas Apr 15, 2021
848f198
Removed comma
mmoschovas Apr 15, 2021
8a72a78
Recommitting changes
mmoschovas Apr 15, 2021
0d9acd0
Updated regex to look for either '|" in macro name. Seems there is a …
mmoschovas Apr 15, 2021
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
13 changes: 10 additions & 3 deletions gulpfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const execa = require('execa');

var prebid = require('./package.json');
var dateString = 'Updated : ' + (new Date()).toISOString().substring(0, 10);
var banner = '/* <%= prebid.name %> v<%= prebid.version %>\n' + dateString + '\nModules: <%= modules %> */\n';
var banner = '/* <%= prebid.name %> v<%= prebid.version %>\n' + dateString + '*/\n';
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason to drop this line? having the list of modules at the top of the prebid code is really useful when debugging code and looking at it as a human

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the issue, we did not want to add more to the JS than necessary by having duplicate lists. So it was removed from the comment to live solely in the array.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My read on the issue was to drop the string so that "machines" could use an array more intelligently than hacking some text; but the text is still important for humans. The way it's implemented now makes it harder for humans to find the list, although still better than prior to the banner being there.

@bretg do you have strong feelings on this? My vote would be to leave the banner AND add the array that was added here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While I appreciate that the array is somewhat harder for a person to read, Prebid.js is constantly under fire for being larger than desired. An average publisher likely uses 5 bid adapters and 3-5 other modules, so the list of module names is likely between 150-200 bytes. Replicating that list seems to drive us away from the goal of making the build package smaller.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While I don't personally think the few extra bytes is a big deal, I can appreciate that some pubs do. Can we at least add a util function that prints the list modules so I can just hit that in console rather than digging for the array in the source code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smenzer I put in the other comment you can add pbjs.installedModules to the console to get the array printed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah you're right sorry I missed that.

var port = 9999;
const FAKE_SERVER_HOST = argv.host ? argv.host : 'localhost';
const FAKE_SERVER_PORT = 4444;
Expand Down Expand Up @@ -134,6 +134,12 @@ function watch(done) {
done();
};

function makeModuleList(modules) {
return modules.map(module => {
return '"' + module + '"'
});
}

function makeDevpackPkg() {
var cloned = _.cloneDeep(webpackConfig);
cloned.devtool = 'source-map';
Expand All @@ -145,6 +151,7 @@ function makeDevpackPkg() {
return gulp.src([].concat(moduleSources, analyticsSources, 'src/prebid.js'))
.pipe(helpers.nameModules(externalModules))
.pipe(webpackStream(cloned, webpack))
.pipe(replace(/('|")v\$prebid\.modulesList\$('|")/g, makeModuleList(externalModules)))
.pipe(gulp.dest('build/dev'))
.pipe(connect.reload());
}
Expand All @@ -157,13 +164,13 @@ function makeWebpackPkg() {

const analyticsSources = helpers.getAnalyticsSources();
const moduleSources = helpers.getModulePaths(externalModules);
const modulesString = getModulesListToAddInBanner(externalModules);

return gulp.src([].concat(moduleSources, analyticsSources, 'src/prebid.js'))
.pipe(helpers.nameModules(externalModules))
.pipe(webpackStream(cloned, webpack))
.pipe(uglify())
.pipe(gulpif(file => file.basename === 'prebid-core.js', header(banner, { prebid: prebid, modules: modulesString })))
.pipe(replace(/('|")v\$prebid\.modulesList\$('|")/g, makeModuleList(externalModules)))
.pipe(gulpif(file => file.basename === 'prebid-core.js', header(banner, { prebid: prebid})))
.pipe(gulp.dest('build/dist'));
}

Expand Down
3 changes: 3 additions & 0 deletions src/prebid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ $$PREBID_GLOBAL$$.libLoaded = true;
$$PREBID_GLOBAL$$.version = 'v$prebid.version$';
utils.logInfo('Prebid.js v$prebid.version$ loaded');

// modules list generated from build
$$PREBID_GLOBAL$$.installedModules = ['v$prebid.modulesList$'];

// create adUnit array
$$PREBID_GLOBAL$$.adUnits = $$PREBID_GLOBAL$$.adUnits || [];

Expand Down