Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add undefinedReplacement option to diffJson #156

Merged
merged 1 commit into from
Dec 22, 2016
Merged

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.

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 = {};
}

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.

None yet

2 participants