Crubin/remove request promise #55

caitlinrubin-optimizely · 2017-05-24T19:04:26Z

No description provided.

…vided event dispatcher

mikeproeng37 · 2017-05-24T22:30:32Z

package.json

@@ -21,15 +21,17 @@
  "homepage": "https://github.com/optimizely/javascript-sdk#readme",
  "dependencies": {
    "es6-promise": "^3.3.1",
-    "optimizely-server-sdk":  "^1.3.0"
+    "optimizely-server-sdk": "^1.3.0",
+    "xmlhttprequest": "^1.8.0"


Is this still needed? If it's for tests, please move it to devDependencies

it's used in event_dispatcher/index.js in the toQueryString method -- should I just copy that over?

I don't see this library xmlhttprequest used in the toQueryString method

O sorry I thought you meant optimizely-server-sdk -- still getting used to reading these git diffs -- you're right I removed it

mikeproeng37 · 2017-05-24T22:30:45Z

package.json

@@ -21,15 +21,17 @@
  "homepage": "https://github.com/optimizely/javascript-sdk#readme",
  "dependencies": {
    "es6-promise": "^3.3.1",


Can get rid of this dependency now

mikeproeng37 · 2017-05-24T22:31:30Z

lib/plugins/event_dispatcher/tests.js

@@ -0,0 +1,92 @@
+/**
+ * Copyright 2016-2017, Optimizely


Should just say 2017

mikeproeng37 · 2017-05-24T22:32:47Z

package.json

  },
  "devDependencies": {
    "chai": "^3.5.0",
    "eslint": "^2.9.0",
    "json-loader": "^0.5.4",
    "mocha": "^2.5.3",
    "sinon": "^1.17.4",
-    "webpack": "^1.13.1"
+    "webpack": "^1.13.1",
+    "lodash": "4.13.1"


please alphabetize these

mikeproeng37 · 2017-05-24T22:34:18Z

tests/test_data.js

@@ -0,0 +1,266 @@
+/**
+ * Copyright 2016-2017, Optimizely


Should just say 2017

mikeproeng37 · 2017-05-24T22:35:50Z

lib/plugins/event_dispatcher/tests.js

+        var callback = sinon.spy();
+        eventDispatcher.dispatchEvent(eventObj, callback);
+        requests[ 0 ].respond([ 200 , {}, '{"url":"https://cdn.com/event","body":{"id":123},"httpVerb":"POST","params":{"testParam":"testParamValue"}}' ]);
+        done();


Once you call done() the funciton will exit, meaning the assertion you are making below won't get executed.

mikeproeng37 · 2017-05-24T22:36:20Z

lib/plugins/event_dispatcher/tests.js

+        var callback = sinon.spy();
+        eventDispatcher.dispatchEvent(eventObj, callback);
+        requests[ 0 ].respond([ 200 , {}, '{"url":"https://cdn.com/event","httpVerb":"GET"' ]);
+        done();


Same as above

mikeproeng37 · 2017-05-25T22:38:59Z

.travis.yml

@@ -9,3 +9,8 @@ branches:
 install: "npm install"
 script:
  - "npm test"
+addons:


We shouldn't include these in the repo, can you revert this?

This should be added to the repo's config settings. We shouldn't check this in!

mikeproeng37 · 2017-05-25T22:39:18Z

CHANGELOG

+* Bump optimizely-server-sdk to version 1.3.1, which includes:
+  - Minor performance improvements.
+-------------------------------------------------------------------------------
+-------------------------------------------------------------------------------


Leave an empty line between these

mikeproeng37 · 2017-05-25T22:39:34Z

CHANGELOG

@@ -1,4 +1,11 @@
 -------------------------------------------------------------------------------
+1.4.1
+-------------------------------------------------------------------------------
+* Switched to karma for testing


We're also getting rid of es6-promise

mikeproeng37 · 2017-05-25T22:40:04Z

karma.conf.js

@@ -0,0 +1,78 @@
+// Karma configuration


This file should be .gitignored, I think, since it is generated

For some reason this file is still showing up

mikeproeng37 · 2017-05-25T22:40:42Z

Gruntfile.js

@@ -0,0 +1,16 @@
+module.exports = function (grunt) {
+    grunt.initConfig({
+        karma: {


Can we configure it to run on chrome by default, instead of Phantom

Please have it run on chrome by default instead of Phantom

mikeproeng37 · 2017-05-25T22:41:11Z

lib/plugins/event_dispatcher/index.js

@@ -1,5 +1,5 @@
 /**
- * Copyright 2016-2017, Optimizely
+ * Copyright 2017, Optimizely


This should say 2016-2017 because the file was created in 2016, and was modified again in 2017.

mikeproeng37 · 2017-05-25T22:41:22Z

lib/plugins/event_dispatcher/index.js

@@ -24,40 +23,36 @@ module.exports = {
   * Sample event dispatcher implementation for tracking impression and conversions
   * Users of the SDK can provide their own implementation
   * @param  {Object} eventObj
-   * @return {Promise<Object>}
+   * @param  {function} callback


{Function}

mikeproeng37 · 2017-05-25T22:42:04Z

webpack.config.js

@@ -1,4 +1,5 @@
 var webpack = require('webpack');
+var path = require('path');


This is not needed anymore

mikeproeng37 · 2017-05-25T22:42:23Z

webpack.config.js

@@ -16,4 +17,12 @@ module.exports = {
    library: 'optimizelyClient',
    libraryTarget: 'umd'
  },
+  module: {


I don't think we need this after upgrading to webpack 2

…ra, firefox

mikeproeng37

Almost there, just a few small changes left.

mikeproeng37 · 2017-05-26T14:07:05Z

karma.conf.js

@@ -0,0 +1,78 @@
+// Karma configuration


For some reason this file is still showing up

mikeproeng37 · 2017-05-26T14:07:56Z

lib/plugins/event_dispatcher/index.js

+      req.setRequestHeader('Content-Type', 'application/json');
+      req.onreadystatechange = function() {
+        if (req.readyState === 4 && callback && typeof callback === 'function') {
+          callback(JSON.stringify(params));


No need to stringify the params here.

mikeproeng37 · 2017-05-26T14:08:15Z

lib/plugins/event_dispatcher/index.js

+      req.open(POST_METHOD, url, true);
+      req.setRequestHeader('Content-Type', 'application/json');
+      req.onreadystatechange = function() {
+        if (req.readyState === 4 && callback && typeof callback === 'function') {


nitpick: Can you make 4 a constant and use the constance instead

mikeproeng37 · 2017-05-26T14:09:15Z

tests.js

@@ -1,5 +1,5 @@
 /**
- * Copyright 2016-2017, Optimizely
+ * Copyright 2017, Optimizely


This should still say 2016-2017

Should still say 2016-2018

Oops 2016-2017

mikeproeng37 · 2017-05-26T14:10:21Z

Gruntfile.js

@@ -0,0 +1,16 @@
+module.exports = function (grunt) {
+    grunt.initConfig({
+        karma: {


Please have it run on chrome by default instead of Phantom

…travis, added coveralls

mikeproeng37

Looks good overall. We should move the travis credentials out of the source

mikeproeng37 · 2017-05-26T22:08:46Z

.travis.yml

@@ -9,3 +9,8 @@ branches:
 install: "npm install"
 script:
  - "npm test"
+addons:


This should be added to the repo's config settings. We shouldn't check this in!

mikeproeng37

Nice, is great to see all the cross browser tests running!

Just a few small things and we are all set!

mikeproeng37 · 2017-05-30T23:43:22Z

tests.js

@@ -1,5 +1,5 @@
 /**
- * Copyright 2016-2017, Optimizely
+ * Copyright 2017, Optimizely


Should still say 2016-2018

mikeproeng37 · 2017-05-30T23:43:35Z

tests.js

@@ -1,5 +1,5 @@
 /**
- * Copyright 2016-2017, Optimizely
+ * Copyright 2017, Optimizely


Oops 2016-2017

mikeproeng37 · 2017-05-30T23:44:54Z

.travis.yml

@@ -1,11 +1,14 @@
 language: node_js
 node_js:
  - "5"
-  - "0.10"
  - "node"


What version is node , does that point to latest node? If so please also add node 6 here

yep node is "the latest stable Node.js release" -- I can add 6

mikeproeng37

LGTM

coveralls · 2017-05-30T23:59:52Z

Changes Unknown when pulling b461c35 on crubin/removeRequestPromise into ** on master**.

coveralls · 2017-05-30T23:59:52Z

Changes Unknown when pulling b461c35 on crubin/removeRequestPromise into ** on master**.

coveralls · 2017-05-30T23:59:52Z

Changes Unknown when pulling b461c35 on crubin/removeRequestPromise into ** on master**.

mikeproeng37 · 2017-06-01T17:26:23Z

karma.bs.conf.js

+
+    // Continuous Integration mode
+    // if true, Karma captures browsers, runs the tests and exits
+    singleRun: false,


This should be true since we are running on the cloud

mikeproeng37 · 2017-06-01T17:26:35Z

karma.bs.conf.js

+
+
+    // enable / disable watching file and executing tests whenever any file changes
+    autoWatch: true,


This should be false as we don't need watching in BS tests

coveralls · 2017-06-01T17:30:25Z

Changes Unknown when pulling 24b1c8b on crubin/removeRequestPromise into ** on master**.

coveralls · 2017-06-01T17:38:24Z

Changes Unknown when pulling 7c2df7f on crubin/removeRequestPromise into ** on master**.

Caitlin added 7 commits May 23, 2017 17:09

added tests for event_dispatcher

e5a631d

cleanup

5af35d5

added test in ./tests.js to check that optlyinstance activates w/ pro…

2d51a8a

…vided event dispatcher

event_dispatcher tests w/o require XML statement

a5d7658

added fake xml to tests.js

4b66d95

cleanup

0da1930

cleanup

1cd47d1

mikeproeng37 suggested changes May 24, 2017

View reviewed changes

Caitlin and others added 9 commits May 24, 2017 16:20

tests moved to karma

bafa9c1

added sauce addon to .travis.yml file

dff7fcc

fake xhr no longer needed in tests.js

0447fb0

cleanup

37e2dd3

cleanup after npm run lint

6e21535

switch to karma for testing, bump node-sdk version to 1.3.1

578fc24

fixes from last commit

c2abcf0

changed version to 1.4.1 in package.json

7c74459

changed javascript_client_version to 1.4.1

34699a2

mikeproeng37 suggested changes May 25, 2017

View reviewed changes

caitlinrubin-optimizely added 3 commits May 25, 2017 15:52

cleaned up package.json devDependencies

377481d

added karma-webpack, took out es6

bb0f154

cross browser testing w/ browserstack set up for iphone5, chrome, ope…

ffc6f69

…ra, firefox

mikeproeng37 suggested changes May 26, 2017

View reviewed changes

caitlinrubin-optimizely added 2 commits May 26, 2017 10:14

fixed changes needed since thurs pull, added ie, edge and safari

2da34bd

removed karma.conf from gitignore, added browserstack credentials to …

af5bc1b

…travis, added coveralls

mikeproeng37 reviewed May 26, 2017

View reviewed changes

caitlinrubin-optimizely added 4 commits May 26, 2017 15:54

took addons out of .travis

a97a9fb

added grunt-cli to package.json, changed tests to include fake xhr again

4521a37

trying to trigger travis build w new package.json

3978669

added grunt-cli to .travis

d6d51b1

caitlinrubin-optimizely closed this May 30, 2017

caitlinrubin-optimizely reopened this May 30, 2017

caitlinrubin-optimizely closed this May 30, 2017

caitlinrubin-optimizely reopened this May 30, 2017

caitlinrubin-optimizely closed this May 30, 2017

caitlinrubin-optimizely reopened this May 30, 2017

caitlinrubin-optimizely added 2 commits May 30, 2017 11:42

Merge branch 'master' into crubin/removeRequestPromise

9f103da

removed 0.10 testing

139227b

mikeproeng37 reviewed May 30, 2017

View reviewed changes

fixed year, added node v6 to travis file

b461c35

mikeproeng37 approved these changes May 30, 2017

View reviewed changes

put browserstack tests into karma.bs.conf.js

94c2554

mikeproeng37 reviewed Jun 1, 2017

View reviewed changes

changed .travis to npm run

24b1c8b

changed single run to true and autowatch to false for karma.bs

7c2df7f

mikeproeng37 merged commit b39740c into master Jun 1, 2017

		@@ -1,4 +1,5 @@
		var webpack = require('webpack');
		var path = require('path');



		// enable / disable watching file and executing tests whenever any file changes
		autoWatch: true,

Crubin/remove request promise #55

Crubin/remove request promise #55

Uh oh!

Conversation

caitlinrubin-optimizely commented May 24, 2017

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!

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!

mikeproeng37 left a comment

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!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 left a comment

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

coveralls commented Jun 1, 2017 •

edited

Loading

coveralls commented Jun 1, 2017 •

edited

Loading