Incorporated code review comments for #691

sendgrid · Jun 13, 2018 · afc4e65 · woodb · Sep 5, 2018 · thinkingserious
1 parent 2abbbce
commit afc4e65
Show file tree

Hide file tree

Showing 10 changed files with 254 additions and 336 deletions.
diff --git a/package.json b/package.json
@@ -13,7 +13,7 @@
     "chai": "^4.0.1",
     "chai-as-promised": "^6.0.0",
     "dirty-chai": "^1.2.2",
-    "eslint": "^3.19.0",
+    "eslint": "^4.19.1",
     "istanbul": "^1.0.0-alpha.2",
     "lerna": "^2.0.0-rc.5",
     "mocha": "^3.4.2",

diff --git a/packages/helpers/classes/mail.js b/packages/helpers/classes/mail.js
@@ -21,7 +21,7 @@ class Mail {
   constructor(data) {
 
     //Initialize array and object properties
-    this._isDynamic = false;
+    this.isDynamic = false;
     this.personalizations = [];
     this.attachments = [];
     this.content = [];
@@ -86,7 +86,7 @@ class Mail {
     this.setMailSettings(mailSettings);
     this.setTrackingSettings(trackingSettings);
 
-    if (this._isDynamic) {
+    if (this.isDynamic) {
       this.setDynamicTemplateData(dynamicTemplateData)
     } else {
       this.setSubstitutions(substitutions);
@@ -171,7 +171,7 @@ class Mail {
     }
 
     if (templateId.indexOf('d-') === 0) {
-      this._isDynamic = true;
+      this.isDynamic = true;
     }
 
     this.templateId = templateId;
@@ -240,7 +240,7 @@ class Mail {
 
     //We should either send substitutions or dynamicTemplateData
     //depending on the templateId
-    if (this._isDynamic && personalization.substitutions) {
+    if (this.isDynamic && personalization.substitutions) {
       delete personalization.substitutions;
     } else if (personalization.dynamicTemplateData) {
       delete personalization.dynamicTemplateData;
@@ -252,7 +252,7 @@ class Mail {
     }
 
     //If this is dynamic, set dynamicTemplateData, or set substitutions
-    if (this._isDynamic) {
+    if (this.isDynamic) {
       this.applyDynamicTemplateData(personalization);
     } else {
       this.applySubstitutions(personalization);
@@ -273,7 +273,7 @@ class Mail {
     ) {
       throw new Error('Provide at least one of to, cc or bcc');
     }
-    this.addPersonalization(new Personalization({ to, cc, bcc }));
+    this.addPersonalization(new Personalization({to, cc, bcc}));
   }
 
   /**
@@ -319,12 +319,12 @@ class Mail {
    */
   applyDynamicTemplateData(personalization) {
     if (personalization instanceof Personalization) {
-      personalization.reverseMergeDynamicTemplateData(this.dynamicTemplateData);
+      personalization.deepMergeDynamicTemplateData(this.dynamicTemplateData);
     }
   }
 
   /**
-   * Set substitutions
+   * Set dynamicTemplateData
    */
   setDynamicTemplateData(dynamicTemplateData) {
     if (typeof dynamicTemplateData === 'undefined') {

diff --git a/packages/helpers/classes/mail.spec.js b/packages/helpers/classes/mail.spec.js
@@ -8,10 +8,10 @@ const Mail = require('./mail');
 /**
  * Tests
  */
-describe('Mail', function () {
+describe('Mail', function() {
 
-  describe('#527', function () {
-    it('shouldn\'t convert the headers to camel/snake case', function () {
+  describe('#527', function() {
+    it('shouldn\'t convert the headers to camel/snake case', function() {
       const mail = new Mail({
         personalizations: [{
           to: '[email protected]',
@@ -40,9 +40,9 @@ describe('Mail', function () {
         .equal('<mailto:[email protected]>');
     });
   });
-  describe('#689', function () {
+  describe('#689', function() {
 
-    it('should detect dynamic template id', function () {
+    it('should detect dynamic template id', function() {
       const mail = new Mail({
         personalizations: [{
           to: '[email protected]',
@@ -58,11 +58,11 @@ describe('Mail', function () {
         content: [{
           type: 'text/plain',
           value: 'test',
-        }]
+        }],
       });
-      expect(mail._isDynamic).to.equal(true);
+      expect(mail.isDynamic).to.equal(true);
     });
-    it('should detect legacy template id', function () {
+    it('should detect legacy template id', function() {
       const mail = new Mail({
         personalizations: [{
           to: '[email protected]',
@@ -78,19 +78,19 @@ describe('Mail', function () {
         content: [{
           type: 'text/plain',
           value: 'test',
-        }]
+        }],
       });
-      expect(mail._isDynamic).to.equal(false);
+      expect(mail.isDynamic).to.equal(false);
     });
-    it('should ignore substitutions if templateId is dynamic', function () {
+    it('should ignore substitutions if templateId is dynamic', function() {
       const mail = new Mail({
         personalizations: [{
           to: '[email protected]',
           headers: {
             'test-header': 'test',
           },
           substitutions: {
-            test2: 'Test2'
+            test2: 'Test2',
           },
           dynamicTemplateData: {
             test2: 'Testy 2',
@@ -102,7 +102,7 @@ describe('Mail', function () {
           test2: 'Test 2',
         },
         substitutions: {
-          test1: 'Test1'
+          test1: 'Test1',
         },
         from: {
           email: '[email protected]',
@@ -112,7 +112,7 @@ describe('Mail', function () {
         content: [{
           type: 'text/plain',
           value: 'test',
-        }]
+        }],
       });
       expect(mail.substitutions).to.equal(null);
       expect(mail.personalizations[0].substitutions).to.deep.equal({});
@@ -121,37 +121,37 @@ describe('Mail', function () {
       expect(mail.personalizations[0].dynamicTemplateData).to.deep.equal({ test1: 'Test 1', test2: 'Testy 2', test3: 'Testy 3' });
 
       expect(mail.toJSON()).to.deep.equal({
-        "content": [
+        'content': [
           {
-            "type": "text/plain",
-            "value": "test"
-          }
+            'type': 'text/plain',
+            'value': 'test',
+          },
         ],
-        "from": {
-          "email": "[email protected]"
+        'from': {
+          'email': '[email protected]',
         },
-        "personalizations": [
+        'personalizations': [
           {
-            "dynamic_template_data": {
-              "test1": "Test 1",
-              "test2": "Testy 2",
-              "test3": "Testy 3"
+            'dynamic_template_data': {
+              'test1': 'Test 1',
+              'test2': 'Testy 2',
+              'test3': 'Testy 3',
             },
-            "headers": {
-              "test-header": "test"
+            'headers': {
+              'test-header': 'test',
             },
-            "to": [
+            'to': [
               {
-                "email": "[email protected]",
-                "name": ""
-              }
-            ]
-          }
+                'email': '[email protected]',
+                'name': '',
+              },
+            ],
+          },
         ],
-        "subject": "test",
-        "template_id": "d-df80613cccc6441ea5cd7c95377bc1ef"
+        'subject': 'test',
+        'template_id': 'd-df80613cccc6441ea5cd7c95377bc1ef',
       });
     });
 
-  })
+  });
 });
diff --git a/packages/helpers/classes/personalization.js b/packages/helpers/classes/personalization.js
@@ -7,7 +7,7 @@ const EmailAddress = require('./email-address');
 const toCamelCase = require('../helpers/to-camel-case');
 const toSnakeCase = require('../helpers/to-snake-case');
 const deepClone = require('../helpers/deep-clone');
-const mergeDataDeep = require('../helpers/merge-data-deep');
+const merge = require('deepmerge');
 const wrapSubstitutions = require('../helpers/wrap-substitutions');
 
 /**
@@ -277,16 +277,16 @@ class Personalization {
   /**
    * Reverse merge dynamic template data, preserving existing ones
    */
-  reverseMergeDynamicTemplateData(dynamicTemplateData) {
+  deepMergeDynamicTemplateData(dynamicTemplateData) {
     if (typeof dynamicTemplateData === 'undefined' || dynamicTemplateData === null) {
       return;
     }
     if (typeof dynamicTemplateData !== 'object') {
       throw new Error(
-        'Object expected for `dynamicTemplateData` in reverseMergeDynamicTemplateData'
+        'Object expected for `dynamicTemplateData` in deepMergeDynamicTemplateData'
       );
     }
-    this.dynamicTemplateData = mergeDataDeep(dynamicTemplateData, this.dynamicTemplateData);
+    this.dynamicTemplateData = merge(dynamicTemplateData, this.dynamicTemplateData);
   }
 
   /**
@@ -314,7 +314,7 @@ class Personalization {
     } = this;
 
     //Initialize with mandatory values
-    const json = { to };
+    const json = {to};
 
     //Arrays
     if (Array.isArray(cc) && cc.length > 0) {

diff --git a/packages/helpers/classes/personalization_specs/PERSONALIZATION-SPECS-README.md b/packages/helpers/classes/personalization_specs/PERSONALIZATION-SPECS-README.md
@@ -16,7 +16,7 @@
 - addSubstitution() - add-substitutions.spec.js
 - reverseMergeSubstitutions() - reverse-merge-substitutions.spec.js
 - setSubstitutionWrappers() - set-substitutions-wrappers.spec.js
-- reverseMergeDynamicTemplateData() - reverse-merge-dynamic_template_data.spec.js
+- deepMergeDynamicTemplateData() - reverse-merge-dynamic_template_data.spec.js
 - toJSON() - to-json.spec.js
 - fromData() - from-data.spec.js
 - #527 - 527-camel-case-headers.spec.js
diff --git a/packages/helpers/classes/personalization_specs/reverse-merge-dynamic_teplate_data.spec.js b/packages/helpers/classes/personalization_specs/reverse-merge-dynamic_teplate_data.spec.js
@@ -18,36 +18,36 @@ describe('Personalization', function () {
   });
 
   //Reverse merge substitutions
-  describe('reverseMergeDynamicTemplateData()', function () {
+  describe('deepMergeDynamicTemplateData()', function () {
     it('should reverse merge dynamicTemplateData', function () {
       p.setDynamicTemplateData({ test1: 'Test1' });
-      p.reverseMergeDynamicTemplateData({ test2: 'Test2' });
+      p.deepMergeDynamicTemplateData({ test2: 'Test2' });
       expect(p.dynamicTemplateData).to.have.a.property('test1');
       expect(p.dynamicTemplateData).to.have.a.property('test2');
       expect(p.dynamicTemplateData.test1).to.equal('Test1');
       expect(p.dynamicTemplateData.test2).to.equal('Test2');
     });
     it('should not overwrite existing keys', function () {
       p.setDynamicTemplateData({ test1: 'Test1' });
-      p.reverseMergeDynamicTemplateData({ test1: 'Test3', test2: 'Test2' });
+      p.deepMergeDynamicTemplateData({ test1: 'Test3', test2: 'Test2' });
       expect(p.dynamicTemplateData).to.have.a.property('test1');
       expect(p.dynamicTemplateData).to.have.a.property('test2');
       expect(p.dynamicTemplateData.test1).to.equal('Test1');
       expect(p.dynamicTemplateData.test2).to.equal('Test2');
     });
     it('should work without prior dynamicTemplateData', function () {
-      p.reverseMergeDynamicTemplateData({ test2: 'Test2' });
+      p.deepMergeDynamicTemplateData({ test2: 'Test2' });
       expect(p.dynamicTemplateData).to.have.a.property('test2');
       expect(p.dynamicTemplateData.test2).to.equal('Test2');
     });
     it('should throw an error for invalid input', function () {
       expect(function () {
-        p.reverseMergeDynamicTemplateData(3);
+        p.deepMergeDynamicTemplateData(3);
       }).to.throw(Error);
     });
     it('should accept no input', function () {
       expect(function () {
-        p.reverseMergeDynamicTemplateData();
+        p.deepMergeDynamicTemplateData();
       }).not.to.throw(Error);
     });
   });

diff --git a/packages/helpers/index.d.ts b/packages/helpers/index.d.ts
@@ -1,7 +1,7 @@
-import * as helpers from "@sendgrid/helpers/helpers/index"
-import * as classes from "@sendgrid/helpers/classes/index"
+import * as helpers from '@sendgrid/helpers/helpers/index';
+import * as classes from '@sendgrid/helpers/classes/index';
 
 export {
   helpers,
   classes,
-};
+};
diff --git a/packages/helpers/package.json b/packages/helpers/package.json
@@ -30,6 +30,7 @@
     "helpers"
   ],
   "dependencies": {
-    "chalk": "^2.0.1"
+    "chalk": "^2.0.1",
+    "deepmerge": "^2.1.1"
   }
 }
diff --git a/packages/helpers/yarn.lock b/packages/helpers/yarn.lock
@@ -26,6 +26,10 @@ color-name@^1.1.1:
   version "1.1.3"
   resolved "https://registry.yarnpkg.com/color-name/-/color-name-1.1.3.tgz#a7d0558bd89c42f795dd42328f740831ca53bc25"
 
+deepmerge@^2.1.1:
+  version "2.1.1"
+  resolved "https://registry.yarnpkg.com/deepmerge/-/deepmerge-2.1.1.tgz#e862b4e45ea0555072bf51e7fd0d9845170ae768"
+
 escape-string-regexp@^1.0.5:
   version "1.0.5"
   resolved "https://registry.yarnpkg.com/escape-string-regexp/-/escape-string-regexp-1.0.5.tgz#1b61c0562190a8dff6ae3bb2cf0200ca130b86d4"