Replace @ndhoule/defaults with ES6 spread syntax merging (segmentio#185)

Author: bryanmikaelian

HISTORY.md

@@ -10,6 +10,10 @@
 
 - Fix Potential DOM-based XSS via prototype pollution
 
+# 4.1.5 / 2020-09-17
+
+- Replace @ndhoule/defaults with merging via ES6 spread syntax
+
 # 4.1.4 / 2020-09-16
 
 - Replace `@ndhoule/includes` with `lodash.includes`

lib/analytics.ts

@@ -5,7 +5,8 @@ import {
   InitOptions,
   SegmentAnalytics,
   SegmentOpts,
-  SegmentIntegration
+  SegmentIntegration,
+  PageDefaults, Message
 } from './types';
 
 import pick from 'lodash.pick'
@@ -324,8 +325,13 @@ Analytics.prototype.identify = function(
   });
 
   // Add the initialize integrations so the server-side ones can be disabled too
+  // NOTE: We need to merge integrations, not override them with assign
+  // since it is possible to change the initialized integrations at runtime.
   if (this.options.integrations) {
-    defaults(msg.integrations, this.options.integrations);
+    msg.integrations = {
+      ...this.options.integrations,
+      ...msg.integrations
+    }
   }
 
   this._invoke('identify', new Identify(msg));
@@ -380,8 +386,13 @@ Analytics.prototype.group = function(
   });
 
   // Add the initialize integrations so the server-side ones can be disabled too
+  // NOTE: We need to merge integrations, not override them with assign
+  // since it is possible to change the initialized integrations at runtime.
   if (this.options.integrations) {
-    defaults(msg.integrations, this.options.integrations);
+    msg.integrations = {
+      ...this.options.integrations,
+      ...msg.integrations
+    }
   }
 
   this._invoke('group', new Group(msg));
@@ -445,10 +456,12 @@ Analytics.prototype.track = function(
   }
 
   // Add the initialize integrations so the server-side ones can be disabled too
-  defaults(
-    msg.integrations,
-    this._mergeInitializeAndPlanIntegrations(planIntegrationOptions)
-  );
+  // NOTE: We need to merge integrations, not override them with assign
+  // since it is possible to change the initialized integrations at runtime.
+  msg.integrations = {
+    ...this._mergeInitializeAndPlanIntegrations(planIntegrationOptions),
+    ...msg.integrations
+  }
 
   this._invoke('track', new Track(msg));
 
@@ -601,8 +614,15 @@ Analytics.prototype.page = function(
 
   // Ensure properties has baseline spec properties.
   // TODO: Eventually move these entirely to `options.context.page`
-  const defs = pageDefaults();
-  defaults(properties, defs);
+  // FIXME: This is purposely not overriding `defs`. There was a bug in the logic implemented by `@ndhoule/defaults`.
+  //        This bug made it so we only would overwrite values in `defs` that were set to `undefined`.
+  //        In some cases, though, pageDefaults  will return defaults with values set to "" (such as `window.location.search` defaulting to "").
+  //        The decision to not fix this bus was made to preserve backwards compatibility.
+  const defs = pageDefaults()
+  properties = {
+    ...properties,
+    ...defs
+  }
 
   // Mirror user overrides to `options.context.page` (but exclude custom properties)
   // (Any page defaults get applied in `this.normalize` for consistency.)
@@ -622,8 +642,13 @@ Analytics.prototype.page = function(
   });
 
   // Add the initialize integrations so the server-side ones can be disabled too
+  // NOTE: We need to merge integrations, not override them with assign
+  // since it is possible to change the initialized integrations at runtime.
   if (this.options.integrations) {
-    defaults(msg.integrations, this.options.integrations);
+    msg.integrations = {
+      ...this.options.integrations,
+      ...msg.integrations
+    }
   }
 
   this._invoke('page', new Page(msg));
@@ -675,8 +700,13 @@ Analytics.prototype.alias = function(
   });
 
   // Add the initialize integrations so the server-side ones can be disabled too
+  // NOTE: We need to merge integrations, not override them with assign
+  // since it is possible to change the initialized integrations at runtime.
   if (this.options.integrations) {
-    defaults(msg.integrations, this.options.integrations);
+    msg.integrations = {
+      ...this.options.integrations,
+      ...msg.integrations
+    }
   }
 
   this._invoke('alias', new Alias(msg));
@@ -975,15 +1005,19 @@ Analytics.prototype._parseQuery = function(query: string): SegmentAnalytics {
  */
 
 Analytics.prototype.normalize = function(msg: {
-  context: { page };
+  options: { [key: string]: unknown }
+  context: { page: Partial<PageDefaults> };
   anonymousId: string;
 }): object {
   msg = normalize(msg, Object.keys(this._integrations));
   if (msg.anonymousId) user.anonymousId(msg.anonymousId);
   msg.anonymousId = user.anonymousId();
 
   // Ensure all outgoing requests include page data in their contexts.
-  msg.context.page = defaults(msg.context.page || {}, pageDefaults());
+  msg.context.page = {
+    ...pageDefaults(),
+    ...msg.context.page
+  };
 
   return msg;
 };

lib/cookie.ts

@@ -10,9 +10,10 @@ import cloneDeep from 'lodash.clonedeep'
 var bindAll = require('bind-all');
 var cookie = require('@segment/cookie');
 var debug = require('debug')('analytics.js:cookie');
-var defaults = require('@ndhoule/defaults');
 var topDomain = require('@segment/top-domain');
 
+const MAX_AGE_ONE_YEAR = 31536000000
+
 /**
  * Initialize a new `Cookie` with `options`.
  *
@@ -32,16 +33,20 @@ Cookie.prototype.options = function(options?: CookieOptions) {
 
   options = options || {};
 
-  var domain = '.' + topDomain(window.location.href);
+  let domain = '.' + topDomain(window.location.href);
   if (domain === '.') domain = null;
 
-  this._options = defaults(options, {
-    // default to a year
-    maxage: 31536000000,
-    path: '/',
+  const defaults: CookieOptions  = {
+    maxage: MAX_AGE_ONE_YEAR,
     domain: domain,
+    path: '/',
     sameSite: 'Lax'
-  });
+  }
+
+  this._options = {
+    ...defaults,
+    ...options
+  };
 
   // http://curl.haxx.se/rfc/cookie_spec.html
   // https://publicsuffix.org/list/effective_tld_names.dat

lib/entity.ts

@@ -10,7 +10,6 @@ import assignIn from 'lodash.assignin'
 
 var cookie = require('./cookie');
 var debug = require('debug')('analytics:entity');
-var defaults = require('@ndhoule/defaults');
 var memory = require('./memory');
 var store = require('./store');
 var isodateTraverse = require('@segment/isodate-traverse');
@@ -74,7 +73,10 @@ Entity.prototype.storage = function() {
 
 Entity.prototype.options = function(options?: InitOptions) {
   if (arguments.length === 0) return this._options;
-  this._options = defaults(options || {}, this.defaults || {});
+  this._options = {
+    ...this.defaults,
+    ...options
+  }
 };
 
 /**

lib/normalize.ts

@@ -6,11 +6,11 @@ import includes from 'lodash.includes'
  */
 
 var debug = require('debug')('analytics.js:normalize');
-var defaults = require('@ndhoule/defaults');
 var type = require('component-type');
 var uuid = require('uuid/v4');
 var md5 = require('spark-md5').hash;
 
+
 /**
  * HOP.
  */
@@ -88,7 +88,11 @@ function normalize(msg: Message, list: Array<any>): NormalizedMessage {
   delete msg.options;
   ret.integrations = integrations;
   ret.context = context;
-  ret = defaults(ret, msg);
+  ret = {
+    ...msg,
+    ...ret
+  }
+
   debug('->', ret);
   return ret;
 

lib/store.ts

@@ -1,11 +1,12 @@
 'use strict';
 
+import { StoreOptions } from './types';
+
 /*
  * Module dependencies.
  */
 
 var bindAll = require('bind-all');
-var defaults = require('@ndhoule/defaults');
 var store = require('@segment/store');
 
 /**
@@ -22,11 +23,14 @@ function Store(options?: { enabled: boolean }) {
  * Set the `options` for the store.
  */
 
-Store.prototype.options = function(options?: { enabled?: boolean }) {
+Store.prototype.options = function(options?: StoreOptions) {
   if (arguments.length === 0) return this._options;
 
   options = options || {};
-  defaults(options, { enabled: true });
+  options = {
+    enabled: true,
+    ...options
+  };
 
   this.enabled = options.enabled && store.enabled;
   this._options = options;

lib/types.ts

@@ -37,6 +37,7 @@ export interface CookieOptions {
   domain?: string;
   path?: string;
   secure?: boolean;
+  sameSite?: string
 }
 
 export interface MetricsOptions {
@@ -46,7 +47,7 @@ export interface MetricsOptions {
   maxQueueSize?: number;
 }
 
-interface StoreOptions {
+export interface StoreOptions {
   enabled?: boolean;
 }
 

package.json

@@ -30,7 +30,6 @@
   },
   "homepage": "https://github.com/segmentio/analytics.js-core#readme",
   "dependencies": {
-    "@ndhoule/defaults": "^2.0.1",
     "@segment/canonical": "^1.0.0",
     "@segment/cookie": "^1.1.5",
     "@segment/is-meta": "^1.0.0",

test/analytics.test.ts

@@ -11,6 +11,7 @@ var pageDefaults = require('../build/pageDefaults');
 var sinon = require('sinon');
 var tick = require('next-tick');
 var trigger = require('compat-trigger-event');
+
 var Identify = Facade.Identify;
 var cookie = Analytics.cookie;
 var group = analytics.group();
@@ -828,6 +829,22 @@ describe('Analytics', function() {
       });
     });
 
+    it('should should not overwrite context.page values due to an existing bug', function() {
+      analytics.page({ prop: true, context: { page: { search: "lol" } } }, { context: { page: { search: "" } } });
+      var page = analytics._invoke.args[0][1];
+      // FIXME: This assert should fail once the bug is fixed
+      assert.notDeepEqual(page.context(), {
+        page: {
+          ...defaults,
+          search: "lol",
+        },
+      });
+      // FIXME: This assert should fail once the bug is fixed
+      assert.deepEqual(page.context(), {
+        page: defaults,
+      });
+    });
+
     it('should emit page', function(done) {
       analytics.once('page', function(category, name, props, opts) {
         assert(category === 'category');
@@ -1608,6 +1625,29 @@ describe('Analytics', function() {
       assert.deepEqual(track.context(), { page: contextPage });
     });
 
+    it('should support overwriting context.page fields', function() {
+      analytics.track(
+        'event',
+        {},
+        {
+          context: {
+            page: {
+              title: 'lol'
+            }
+          }
+        }
+      );
+
+      var track = analytics._invoke.args[0][1];
+      assert.deepEqual(
+        track.context().page,
+        {
+          ...contextPage,
+          title: 'lol'
+        }
+      );
+    });
+
     it('should accept context.traits', function() {
       analytics.track('event', { prop: 1 }, { traits: { trait: true } });
       var track = analytics._invoke.args[0][1];

test/cookie.test.ts

@@ -63,6 +63,15 @@ describe('cookie', function() {
       assert(cookie.options().maxage === 31536000000);
     });
 
+    it('should have default options', function() {
+      cookie.options({ domain: '' });
+
+      assert(cookie.options().maxage === 31536000000);
+      assert(cookie.options().path === '/');
+      assert(cookie.options().domain === '');
+      assert(cookie.options().sameSite === 'Lax');
+    });
+
     it('should set the domain correctly', function() {
       cookie.options({ domain: '' });
       assert(cookie.options().domain === '');

test/normalize.test.ts

@@ -64,6 +64,18 @@ describe('normalize', function() {
         }
       });
     });
+
+    it('should merge with defaults', function() {
+      opts.context = { foo: 5 };
+      var out = normalize(msg, list);
+      assert.deepEqual(out.integrations, {});
+      assert.deepEqual(out.context, { foo: 5 });
+
+      msg.options = { integrations: { Segment: true }, context: { foo: 6 } };
+      out = normalize(msg, list);
+      assert.deepEqual(out.integrations, { Segment: true });
+      assert.deepEqual(out.context, { foo: 6 });
+    });
   });
 
   describe('integrations', function() {

test/store.test.ts

@@ -43,5 +43,11 @@ describe('store', function() {
       assert(store.options().enabled === false);
       assert(store.enabled === false);
     });
+
+    it('should have default options', function() {
+      store.options({});
+
+      assert(store.options().enabled);
+    });
   });
 });

yarn.lock

@@ -5969,6 +5969,7 @@ lodash-es@^4.17.11:
 lodash.assign@^4.2.0:
   version "4.2.0"
   resolved "https://registry.yarnpkg.com/lodash.assign/-/lodash.assign-4.2.0.tgz#0d99f3ccd7a6d261d19bdaeb9245005d285808e7"
+  integrity sha1-DZnzzNem0mHRm9rrkkUAXShYCOc=
 
 lodash.assignin@^4.2.0:
   version "4.2.0"