PLTX-2337 (refactor) upgrade to latest CMP from appnexus by potench · Pull Request #4 · Openmail/system1-cmp

potench · 2018-06-29T20:04:10Z

Background

This is work in progress

Move CMP to openmail
Upgrade CMP to latest from appnexus
Version the s1.cmp.js in dist folder based on package.json
Deploy the reference and doc files to the root
Update readme for new CMP deployment and references
~~Replace core-js with babel plugin for IE11 polyfill support~~ see useBuiltIns: figure out how to remove polyfills that aren't actually used in the codebase babel/babel-preset-env#84 Adds too much bloat, would like to add packageSize module first

Test Plan

cd system1-cmp
yarn
yarn start
# http://localhost:5000/reference.html should load S1 CMP
# http://localhost:5000/ should load Original CMP

yarn build:s1 
# should build to dist/ folder based on package.json version

yarn test
# all tests should pass

jamsea · 2018-07-03T20:41:13Z

-				callback
-			});
-		}
+		// else if (


Is there a reason we commented this out instead of erasing it? Usually commenting out chunks is a bit of a code smell

I left it commented out because the upstream CMP has a "todo" to remove this, since i've already removed it i want the comment here so it matches up with the upstream file and context is given

jamsea · 2018-07-03T20:42:49Z

@@ -0,0 +1 @@
+8.11.3


jamsea · 2018-07-03T20:45:15Z

+// import CopyWebpackPlugin from 'copy-webpack-plugin';
+import autoprefixer from 'autoprefixer';
+import path from 'path';
+// import UglifyJS from 'uglify-es';


Let's erase the commented out imports

jamsea · 2018-07-03T20:54:05Z

-    "clean": "rm -rf build",
-    "dev": "cross-env NODE_ENV=development webpack-dev-server --inline --hot --progress",
-    "start": "serve build -s -c 1",
+    "clean": "rm -rf dist",


yarn add -D rimraf and replace this with:

"clean": "rimraf dist",

This allows the command to work cross platform (mac/linux/bsd/windows)

Actually we already have rimraf so you don't even need to add it

jamsea · 2018-07-03T21:04:53Z

@@ -0,0 +1,135 @@
+// __cmp('setConsentUiCallback', callback) QUANTCAST
+import 'core-js/fn/array/find-index';


Using this library in one of our sites will double up the bundle size for anything we're using babel-polyfill for. I think what we want to do is something like this:
https://stackoverflow.com/questions/33380063/what-is-the-best-way-to-include-babel-polyfill-using-multiple-entry-points

In development mode in webpack we'd include the polyfill but in production mode we wouldn't include it

Another reference is here:
https://babeljs.io/docs/en/babel-polyfill.html#usage-in-node-browserify-webpack

Im not using babel-polyfill because it added another 130k to the package. But, would be nice to use it, ill make a ticket (see test plan)

jamsea · 2018-07-03T21:07:05Z

@@ -0,0 +1,73 @@
+/* eslint-disable max-nested-callbacks */


^ Whenever I see this I know a party's coming soon

jamsea · 2018-07-03T21:20:01Z

+				'process.env.NODE_ENV': JSON.stringify(ENV)
+			}),
+			new webpack.ProvidePlugin({
+				Promise: 'promise-polyfill'


The babel-polyfill tweak should allow you to remove this

(see test plan) i removed babel-polyfill because it bloats the library (by 130k), will make a ticket to figure out how separate out the polyfills or optimize this

potench · 2018-07-03T22:09:39Z


 			// Expose `processCommand` as the CMP implementation
-			window[CMP_GLOBAL_NAME] = cmp.processCommand;
+			// window[CMP_GLOBAL_NAME] = cmp.processCommand;


same, leaving this comment in because i want to call this out when doing another upstream merge.

potench added 11 commits June 29, 2018 09:04

(refactor) partial migration

b6e4a13

(migrate) upgrade to latest cmp and move to openmail repo

afd905d

Merge branch 'master' into migration

363f923

(refactor) fix webpack dev server for s1

60cc951

(refactor) add cross-var and update deploy:s1 npm scripts

7ac2e22

(feat) deploy:s1

4987361

(refactor) move to configure dir

023c6c9

(refactor) reorganize files

5054fb1

(refactor) deploy s1 and deploy original separately to s3 bucket

a77e5d1

(refactor) reogranize loader files and update deploy

faa5e22

(feat) added .node_version

783ed13

jamsea reviewed Jul 3, 2018

View reviewed changes

jamsea approved these changes Jul 3, 2018

View reviewed changes

potench commented Jul 3, 2018

View reviewed changes

(refactor) remove codesmell from package json and babel config

e42792b

potench merged commit 04052cd into master Jul 3, 2018

potench deleted the migration branch July 3, 2018 22:28

		@@ -0,0 +1,135 @@
		// __cmp('setConsentUiCallback', callback) QUANTCAST
		import 'core-js/fn/array/find-index';

		@@ -0,0 +1 @@
		8.11.3

Conversation

potench commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

potench commented Jun 29, 2018 •

edited

Loading