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

Check if the two operands are of the same type before interpreting a missing prop as a remove op #205

Merged
merged 6 commits into from
Mar 27, 2019

Conversation

alshakero
Copy link
Collaborator

Fixes #31

@alshakero alshakero requested review from tomalec and warpech June 2, 2018 21:07
@alshakero alshakero changed the title Low prio: check if the two operands are of the same type before comparing them deeply Low prio: check if the two operands are of the same type before interpreting a missing prop as a remove op Jun 4, 2018
]);

});
it('Replacing a deep array with an object should be handled well', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's rather an array nested in an object, than deep array, as it's only shallow - one level array of primitives.

]);

});
it('Replacing a two-level deep array with an object should be handled well', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -217,9 +219,12 @@ function _generate(mirror, obj, patches, path) {
}
}
}
else {
else if(Array.isArray(mirror) === Array.isArray(obj)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add some code comments about what's happening here? Why for two arrays or two non-arrays it's a delete, but for array and non-array, it's a replace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add something of the effect of: Because the replace is for the whole obj, not for obj[key].

@robertdumitrescu
Copy link

robertdumitrescu commented Oct 21, 2018

When will be this merged? I created a test in mocha and the array replacement is not the only problem.
`
it('should getDiff and by applying the diff changes to get the modified object', async () => {

        let firstNestedObject = [
            {
                title: 'Products',
                title2: 'Products2',
                children: [
                    {
                        title: 'Electronics',
                        children: [
                            {
                                title: 'Audio-Video',
                                children: [
                                    {
                                        title: 'Game consoles',
                                        children: [
                                            {
                                                title: 'Xbox 360',
                                                date: new Date(2018, 11, 24, 10, 33, 30)
                                            },
                                            {
                                                title: 'PlayStation 4',
                                            },
                                            {
                                                title: 'Sega',
                                            }
                                        ]
                                    }
                                ]
                            },
                            {
                                title: 'Mobile devices'
                            }
                        ]
                    },
                    {
                        title: 'Cars'
                    }
                ]
            },
            {
                title: 'News',
                randomDate: new Date(2018, 11, 24, 10, 33, 30),
                children: [
                    {
                        title: 'Technology',
                        children: [
                            {
                                title: 'CES 2018'
                            },
                            {
                                title: 'Why PHP sucks'
                            }
                        ]
                    },
                    {
                        title: 'Environment',
                        subObj: {
                            prop1: 'randomProp'
                        }
                    }
                ]
            }
        ];


        let secondModifiedNestedObject = [
            {
                title: 'Products Modified - =====>HEEEREE',
                title2: false,
                children: [
                    {
                        title: 'Electronics',
                        children: [
                            {
                                title: 'Audio-Video',
                                extraProp: 1,
                                extraProp00: 1413431431431413,
                                extraProp2: [
                                    'bla',
                                    {
                                        extraSubSneakyProp: 'Super Sneaky Prop =====> HEEEERE'
                                    }
                                ],
                                children: [
                                    {
                                        title: 'Game consoles',
                                        children: [
                                            {
                                                title: 'Xbox 360',
                                                date: null
                                            },
                                            {
                                                title22: 'Sega',
                                            }
                                        ]
                                    }
                                ]
                            },
                            {
                                title: 'Mobile devices'
                            }
                        ]
                    },
                    {
                        title: true
                    }
                ]
            },
            {
                title: 'News',
                randomDate: new Date(2018, 11, 9, 10, 36, 30),
                children: [
                    {
                        title: null,
                        children: [
                            {
                                title: 'CES 2018'
                            },
                            {
                                title: 'Why PHP sucks'
                            },
                            {
                                title: 'Some new article,',
                                customSUbArray: [
                                    {
                                        subObject: 'nananana',
                                        _weirdProp: 'nanana2'
                                    }
                                ]
                            }
                        ]
                    },
                    {
                        title: 'Environment',
                        subObj: [
                            1, 3, 5
                        ]
                    }
                ]
            }
        ];

        let differences = GenericObjectHelper.getRFC6902Diff(firstNestedObject, secondModifiedNestedObject);
        let actualModifiedObject = GenericObjectHelper.applyRFC6902Diff(firstNestedObject, differences);

        expect(actualModifiedObject).to.deep.equal(secondModifiedNestedObject);
    });

`

And it fails on both getting the diff out of a Date object and also when an object is replace with an array.
I am currently doing a spike for this package since we need to develop something similar to git versioning but for Elastic Search documents. Any update?

P.S. get and apply are literally just wrappers around functions.

`
static getRFC6902Diff(firstObject, secondObject) {
return Jsonpatch.compare(firstObject, secondObject);
}

static applyRFC6902Diff(object, differences) {
    let patchedObject = Jsonpatch.applyPatch(object, differences).newDocument;
    return patchedObject;
}

`

@robertdumitrescu
Copy link

Ah... I saw your comments in another issue about Date objects. Fair enough. Is not the focus of this plugin. Apologies. But the array/object replace is a must have though.

@alshakero alshakero merged commit d796de0 into master Mar 27, 2019
@alshakero alshakero deleted the 'Fix-31' branch March 27, 2019 13:51
@alshakero alshakero changed the title Low prio: check if the two operands are of the same type before interpreting a missing prop as a remove op Check if the two operands are of the same type before interpreting a missing prop as a remove op Apr 1, 2019
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.

3 participants