Skip to content

Add undefinedReplacement option to diffJson#156

Merged
kpdecker merged 1 commit intokpdecker:masterfrom
ewnd9:patch-1
Dec 22, 2016
Merged

Add undefinedReplacement option to diffJson#156
kpdecker merged 1 commit intokpdecker:masterfrom
ewnd9:patch-1

Conversation

@ewnd9
Copy link
Contributor

@ewnd9 ewnd9 commented Dec 8, 2016

Fixes #155

Please, let me know if I need to change styling / rename undefinedReplacement to something more concise / anything else.

src/diff/json.js Outdated
options = {};
}

jsonDiff.undefinedReplacement = options.undefinedReplacement;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to assign undefinedReplacement directly to jsonDiff because this.options is overriden each time at the base#diff method (https://github.com/kpdecker/jsdiff/blob/master/src/diff/base.js#L10).

Copy link
Owner

Choose a reason for hiding this comment

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

Humm. This is not ideal as it causes side effects in the global space. Did you try to pass options through?

]);
});

it('should handle callback', function(done) {
Copy link
Contributor Author

@ewnd9 ewnd9 Dec 8, 2016

Choose a reason for hiding this comment

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

I had to add this to keep coverage at 100% which was dropped due to the code below

if (typeof options === 'function') {
  callback = options;
  options = {};
}

src/diff/json.js Outdated
options = {};
}

jsonDiff.undefinedReplacement = options.undefinedReplacement;
Copy link
Owner

Choose a reason for hiding this comment

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

Humm. This is not ideal as it causes side effects in the global space. Did you try to pass options through?

@ewnd9
Copy link
Contributor Author

ewnd9 commented Dec 22, 2016

My apologies for distraction, it's indeed working with just options.

If I recall correctly, I was trying to keep signature of jsonDiff.diff(oldObj, newObj, callback) but now I see it is already handled at https://github.com/kpdecker/jsdiff/blob/master/src/diff/base.js#L6-L9


I've pushed a new commit with changes

@kpdecker kpdecker merged commit 2f92dae into kpdecker:master Dec 22, 2016
@ewnd9
Copy link
Contributor Author

ewnd9 commented Dec 22, 2016

@kpdecker Could you release a new version on npm, please?

@kpdecker
Copy link
Owner

Released in 3.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants