Tixit / odiff

Gets a list of differences between two javascript values.
MIT License
88 stars 13 forks source link

Add "apply" methods implementation #8

Open thenikso opened 5 years ago

thenikso commented 5 years ago

Hi!

First of all thank you for odiff, the differences it produces saved my day at work :D

I found myself needing to apply a diff array to an object and I made the implementation in this PR.

Before adding tests and what not I wanted to ask why odiff doesn't have an apply already and if the implementation in here makes sense.

Thanks!

fresheneesz commented 5 years ago

Glad you find it useful! I didn't create an apply method because I think most of my uses of odiff in my code just didn't need it.

The code you wrote makes sense. The only problem I'm seeing is that there's no way to add items to an array - only to replace the array with an array copy that has those items. If there's any reference to the original array, that reference will no longer point into that object. This is inconsistent with how you've defined applying rm, where it is actually mutating the pointed-to array. I'd recommend changing how you're applying add to splice items into the array (rather than changing rm to be consistent with how you've done add). If someone wants to have behavior that replaces arrays, I think its best that they define that separately either by copying the whole object, then applying the diff on it or by some more granular technique.

I think exposing only applyAll makes things simpler (and rename it to apply), since you can always do odiff.apply([singleDiff], obj) if you need to - only two additional characters. But I like how you've separated the methods and think those should be kept separate internally.

I'd also recommend separating the path-traversal code so that can be exposed. That would make it easier for people to do any kind of more granular things like replacing arrays you add items to (like your current implementation does).

In addition to the above things, before I accept the CL, it needs unit tests and documentation. Would you mind adding that?

I actually do have code like your path traversal code (in my private project where I used odiff) in the form of a class called DynamicPointer that has methods get, set, and unset (add and rm were be done by getting the value and then operating on the resulting array). It also had an extend method that returned a DynamicPointer to a deeper path. Here's the code for that:

// a pointer to a value inside an object
// allows you to dynamically choose what object to point through (the object in which to find the value via the pointer path)
// this can be used for values that don't yet exist - it will dynamically find the inner property at the time of access
var DynamicPointer = exports.DynamicPointer = proto(function() {

    // constructor
    // parameters
        // properties... - each vararg can either be:
            // a single property (e.g. 'a')
            // a string with properties separated by the dot operator (e.g. 'a.b.c')
            // an array with elements matching any of these three (e.g. ['a','b.c', ['d.e', 'f']])
        // e.g. Pointer(obj, 'a.b.c', "d.e", ['f.g', 'h']) points to obj.a.b.c.d.e.f.g.h
    // see normalizePropertyList for related examples
    this.init = function(/*properties...*/) {
        if(arguments.length > 0) {
            var properties = trimArgs(arguments)
            this.path = normalizePropertyList(properties)
        } else {
            this.path = []
        }
    }

    // returns the value pointed to
    // obj is the object to point through
    this.get = function(obj) {
        return evalPointer(this, obj)
    }

    // sets the value pointed to
    // obj is the object to point through
    this.set = function(obj, value) {
        evalPointer(this, obj, 'set', value)
    }

    // obj is the object to point through
    this.unset = function(obj) {
        evalPointer(this, obj, 'unset')
    }

    // returns a new pointer that points to somewhere deeper inside the object this pointer points to
    this.extend = function(properties) {
        var result = DynamicPointer(this.obj)
        result.path = this.path.concat(normalizePropertyList(arguments))

        return result
    }

    function evalPointer(pointer, obj, action, value) {
        var properties = pointer.path

        var lastIndex = properties.length-1
        var cur = obj
        for(var n=0; n<lastIndex; n++) {
            cur = cur[properties[n]]
        }

        if(action === 'set') {
            cur[properties[lastIndex]] = value
        } else if(action === 'unset') {
            delete cur[properties[lastIndex]]
        } else {
            return cur[properties[lastIndex]]
        }
    }
})

Note that proto is fresheneesz/proto. Also note that in that code ^, I was using a string like "a.b.c" to denote paths, but I have a todo to change to using arrays of paths instead of strings, so I don't actually think doing it that way was a good idea after all. normalizePropertyList is what transforms the strings into the type of array path odiff uses. Feel free to use that code above if you want.

Thanks for contributing!

thenikso commented 5 years ago

Hi! thanks for the detailed reply!

As I am using odiff in a Vue project i eventually had to modify the code in this PR to use Vue.set and this made me thing that a shareable "apply" is not that straightforward.

Do you think @fresheneesz that we should continue this PR or perhaps close it until the next good soul comes along?

Best

fresheneesz commented 5 years ago

I'm happy keeping this PR open until its finished or we decide its not worth doing. It might be useful to build something that makes it easier to construct custom "apply" functions, so you can apply a diff to something other than a basic object. I'm not sure how that would look tho. I'll leave it your judgement as to whether or not you think this PR is worth continuing.