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

Modernizing build dependencies #1355

Merged
merged 28 commits into from
Jul 17, 2017
Merged

Modernizing build dependencies #1355

merged 28 commits into from
Jul 17, 2017

Conversation

dbemiller
Copy link
Contributor

@dbemiller dbemiller commented Jul 7, 2017

Type of change

Build upgrades & maintenance

Description of change

Lots of build maintenance relating to webpack, istanbul, and karma.

Fixed bugs

  • Failed tests now use the correct line numbers in the stack traces
  • Code coverage reports use pre-transpiled code now
  • Output from gulp test —browserstack is much more concise

Maintenance

  • Many dependencies (notably Karma and Webpack) run the latest versions
  • CI now uses Node 7 (for Webpack 3.0 compatibility. We should replace yarn with Node 8 in a separate PR)
  • Added gulp test-coverage command. gulp test no longer generates coverage. gulp run-tests uses gulp test-coverage, while gulp serve uses gulp test.
  • PhantomJS replaced by HeadlessChrome everywhere
  • IE9 removed from the browserstack list (since we don't support it anymore)

Performance Comparison

These were very un-scientific... I only ran each test once, and didn't pay attention to my machine load. I can run more careful trials if anyone really cares though.

Old build (from Master)

prebid.js bundle size — 269KB

npm install — 31s
yarn install — 20.45s
gulp build — 13s
gulp lint — 6s
gulp run-tests — 1m1s

New build (from this branch)

prebid.js bundle size — 264KB

npm install — 25s
yarn install — 18.59s
gulp build — 15s
gulp lint — 5s
gulp test — 37s
gulp run-tests — 1m17s (no longer used by gulp serve)
gulp test --browserstack -- About 10 minutes

var webpack = require('webpack-stream');
var path = require('path');
var webpack = require('webpack');
var webpackStream = require('webpack-stream');
Copy link
Collaborator

@protonate protonate Jul 10, 2017

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 require both webpack and webpackStream? Docs seem to indicate stream is a drop-and-replace.

Copy link
Contributor Author

@dbemiller dbemiller Jul 12, 2017

Choose a reason for hiding this comment

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

It's under the Usage header.

If you would like to use a different version of webpack than the one this plugin uses, pass in an optional 2nd argument

They're using webpack 1.12.9.

@mkendall07 mkendall07 requested a review from snapwich July 13, 2017 13:48
@mkendall07
Copy link
Member

@snapwich appreciate your review on these changes.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM. nice work!

@matthewlane matthewlane merged commit 4b04890 into master Jul 17, 2017
@mkendall07 mkendall07 deleted the upgrade-build-dependencies branch July 18, 2017 15:21
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Jul 28, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (95 commits)
  Specify --browsers when using gulp test --watch (prebid#1420)
  Added aliases for aol adapter. (prebid#1371)
  Added MobFox Adapter (prebid#1312)
  Fixed style error. (prebid#1419)
  Add native support for Criteo adapter (prebid#999)
  Update admediaBidAdapter.js (prebid#1395)
  Increment pre version (prebid#1413)
  Prebid 0.26.1 Release (prebid#1412)
  fix prebid#1410 - issue with ie and xhr.timeout (prebid#1411)
  Lint modules directory (prebid#1404)
  Set outstream mediaType based on renderer in response (prebid#1391)
  Fixing the BidAdjustmentEvent fire time (prebid#1399)
  Fix banner showing up in prebid-core.js (prebid#1388)
  Mention NodeJS 4.0 dependency in the README (prebid#1386)
  Increment pre version (prebid#1385)
  Prebid 0.26.0 Release (prebid#1384)
  PulsePoint Lite adapter - adding createNew method for aliasing. (prebid#1383)
  Modernizing build dependencies (prebid#1355)
  StickyAdsTV bidder adapter update (prebid#1311)
  Added CPM value as parameter for Vertoz Adapter (prebid#1306)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants