Skip to content

Commit

Permalink
setSubstitutions and reverseMergeSubstitutions should accept null
Browse files Browse the repository at this point in the history
  • Loading branch information
thinkingserious committed Aug 25, 2017
1 parent 74e3270 commit 3d0090f
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 12 deletions.
6 changes: 3 additions & 3 deletions packages/helpers/classes/personalization.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ class Personalization {
if (typeof substitutions === 'undefined') {
return;
}
if (typeof substitutions !== 'object' || substitutions === null) {
if (typeof substitutions !== 'object') {
throw new Error('Object expected for `substitutions`');
}
this.substitutions = substitutions;
Expand All @@ -247,8 +247,8 @@ class Personalization {
if (typeof substitutions === 'undefined') {
return;
}
if (typeof substitutions !== 'object' || substitutions === null) {
throw new Error('Object expected for `substitutions`');
if (typeof substitutions !== 'object') {
throw new Error('Object expected for `substitutions` in reverseMergeSubstitutions');
}
this.substitutions = Object.assign({}, substitutions, this.substitutions);
}
Expand Down
14 changes: 7 additions & 7 deletions packages/helpers/classes/personalization.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -406,11 +406,11 @@ describe('Personalization', function() {
});
it('should throw an error for invalid input', function() {
expect(function() {
p.setSubstitutions('Invalid');
}).to.throw(Error);
expect(function() {
p.setSubstitutions(null);
p.setSubstitutions(3);
}).to.throw(Error);
//expect(function() {
//p.setSubstitutions(null);
//}).to.throw(Error);
});
it('should accept no input', function() {
expect(function() {
Expand Down Expand Up @@ -480,9 +480,9 @@ describe('Personalization', function() {
expect(function() {
p.reverseMergeSubstitutions(3);
}).to.throw(Error);
expect(function() {
p.reverseMergeSubstitutions(null);
}).to.throw(Error);
//expect(function() {
//p.reverseMergeSubstitutions(null);
//}).to.throw(Error);
});
it('should accept no input', function() {
expect(function() {
Expand Down
4 changes: 2 additions & 2 deletions packages/mail/USE_CASES.md
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ I hope you are having a great day in -city- :)
```js
const sgMail = require('@sendgrid/mail');
sgMail.setApiKey(process.env.SENDGRID_API_KEY);
sgMail.setSubstitutionWrappers('-', '-'); // Configure the substitution tag wrappers globally
sgMail.setSubstitutionWrappers('{{', '}}'); // Configure the substitution tag wrappers globally
const msg = {
to: 'recipient@example.org',
from: 'sender@example.org',
Expand All @@ -245,7 +245,7 @@ Alternatively, you may specify the substitution wrappers via the msg object as w
```js
const msg = {
...
substitutionWrappers: ['-', '-'],
substitutionWrappers: ['{{', '}}'],
...
};
```
Expand Down

6 comments on commit 3d0090f

@adamreisnz
Copy link
Contributor

@adamreisnz adamreisnz commented on 3d0090f Aug 25, 2017

Choose a reason for hiding this comment

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

@thinkingserious You didn't add the || substitutions === null portion back to the first if statement (after undefined check)

@thinkingserious
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed it.

@adamreisnz
Copy link
Contributor

Choose a reason for hiding this comment

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

Saw it, I think you missed it in one location though. It was in two places that I had added that.

@thinkingserious
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, checking

@thinkingserious
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamreisnz
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, in setSubstitutions it just falls through and gets set as null 👍

Please sign in to comment.