Bug: K() breaks references when mixing vanilla objects with observables

[rikutiira]

Example of unexpected behavior:

var o = {}

K(o, o_ => console.log(o === o_)) //true
K(o, 'test', o_ => console.log(o === o_)) //true
K(Kefir.constant(o), o_ => console.log(o === o_)).onValue(() => {}) //true
K(Kefir.constant(o), Kefir.constant('test'), o_ => console.log(o === o_)).onValue(() => {}) //true
K(o, Kefir.constant('test'), o_ => console.log(o === o_)).onValue(() => {}) //false

[rikutiira]

I'm guessing the cause is the object copying here?

function combine(template, values, state) {
  if (template instanceof Observable) {
    return values[++state.index]
  } else if (isArray(template)) {
    const n = template.length
    const next = Array(n)
    for (let i=0; i<n; ++i)
      next[i] = combine(template[i], values, state)
    return next
  } else if (isObject(template)) {
    const next = {}
    for (const k in template)
      next[k] = combine(template[k], values, state)
    return next
  } else {
    return template
  }
}

I think it'd make sense for objects with no observables in them to always be references or then to always be copies, right now it's confusing since it's working in two different ways depending on how K is used.

Personally I'd prefer for combine to check if the object has any observables or not so that you can do reference checks using the variable which K returns. And it probably has less overhead than always copying objects.

[polytypic]

Ah... That is good point! I think originally the idea was only to produce structurally equal object/array structure, but I agree that it would be better to keep object identities intact whenever possible. Like you say, doing so could also improve "systemic" performance as it could be used to avoid some unnecessary allocations and reduce GC pressure. (IIRC, there is some ad-hoc logic in karet that does stuff like that, BTW.)