Final target prop set to {} first, doesn't place nice with reactive setter

[dkebler]

Took me awhile to track this down to your package :).

I am using set-value to set a setter that's calling a rxjs next like so.

    Object.defineProperty(parent, name, {
      configurable: true,
      enumerable: true,
      get () {
        return rx.value
      },
      set (value) {
        rx.value = value
        rx.obs.next({value:value, path:path})
        self.emit(opts.event || self._rx_.event,{value:value, path:path})
        self.emit(path,value)
        self._rx_.hooks.forEach(hook=>hook.call(self,{value:value, path:path}))
      }
    })

In your code the "final" target[prop] (not the parent) is assigned a {} which after call to result is reset to the passed value. Normally no one would notice this double assignment except of course if you happen to be observing that assignment via a setter and a subscription.

    if (!isObject(target[prop])) {
      target[prop] = {};
    }

    if (i === len - 1) {
      result(target, prop, value, merge);
      break;
    }

it looks like this in my observer subscription handler an throws errors down th e line in my code cause it was expecting a primitive value not {}.

observed set value { value: {}, path: 'some.thing' }
observed set value { value: 'value that was to be set', path: 'some.thing' }

So I simply flopped the order

    if (i === len - 1) {
      result(target, prop, value, merge)
      break
    }

    if (!isObject(target[prop] && len)) {
      console.log('not object, set empty')
      target[prop] = {}
    }

and voila no assignment to {} first. Just the one firing of my observer.

Doesn't seem like this would cause any other issue in doing this and will eliminate an unnecessary conditional and assignment.

I'll fork, make this change, run your tests and submit pr.

also kinda wondering if
if (merge && isPlain(target[path]) && isPlain(value)) {
might not work as intended if isPlain doesn't resolve the setter to see if it contains a plain object or not. (i.e it thinks it's not cause it's a setter)

[dkebler]

still passes all your tests with that swap. Now trying to cook up an observable test to add.

[dkebler]

pr here #24

[jonschlinkert]

closing since this was addressed by #24, thanks again @dkebler