fix(path-set): do not return undefined values in toJSON function #117

tadejgolobic commented 3 years ago

High Level Overview of Change

Return value of method toJSON in Hop class in path-set.ts was changed to include only defined values.

Context of Change

Refactoring Hop, Path, and PathSet to extends serialized type and constructor parameters to be of type Buffer, to match how the base class is constructed. Moved from makeClass() -> class.

Type of Change

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Refactor (non-breaking change that only restructures code)
[ ] Tests (You added tests for code that already exists, or your new feature included in this PR)
[ ] Documentation Updates
[ ] Release

Before / After

Before:

toJSON method returnes undefined values. Undefined value is not a valid json value per official JSON standard (ECMA-404, Section 5).

A JSON value can be an object, array, number, string, true, false, or null.

This issue also causes regression in ripple-lib as described in #117.

After:

Revert https://github.com/ripple/ripple-binary-codec/pull/96/files#diff-b819aca034c388228b6db529f7fa97223e2294ff6036f5f9f7bdadd1d793c1eaL115

Return only defined values. If value is undefined simply do not return it. It will still be undefined :)

Test Plan

No tests added.

natenichols commented 3 years ago

Hi @tadejgolobic. Thanks for your contribution.

Nice catch! Your fix in path-set.ts looks good at first glance. I'll pull down your branch and try it out.

Would you refactor the PR to not commit all the files from the ./dist directory. The prepare command should build the library when it is installed from npm with either yarn/npm, and as such its not required to include the ./dist directory in this PR.

tadejgolobic commented 3 years ago

Hi @natenichols thank you for your reply. Yes, I was playing with some stuff and I forgot about this pull request. I have updated now this PR so that only path-set.ts change is included in this PR.

tadejgolobic commented 3 years ago

Thank you for approval. Bare in mind, that I do not have write access to this repo, so I cannot merge this PR. I would be very happy if you can merge this PR. Thank you @natenichols

ripple / ripple-binary-codec