Implemented toJS in such a way that it only recurses into observables

[mweststrate]

This PR fixes #566 / #568. Previously toJS made a deep clone of any javascript structure. This PR changes the behavior; it only clones observables, but leaves all other objects as is. I think that is what most people would expect from this function (I would)

For backward compatibility, people relying on the old behavior can use toJSlegacy (deprecated). The old behavior can probably be simulated by using e.g. lodash: const clone = deepClone(toJs(observable)). (Maybe toJS is not even needed there)

Some questions:

1. for observable objects, should non-observable keys be copied / kept (current implementation: yes)
2. if there is an observable object, deep inside an observable object, with some non observables in between, the deep observable object won't be seen by toJS and hence not converted. Is that a problem? (I think it is a rare case)

So for example

    function MyClass() {}
    const a = new MyClass()
    const b = mobx.observable({ x: 3 })
    const c = mobx.observable({ a: a, b: b })

    t.ok(mobx.toJS(c).a === a) // true
    t.ok(mobx.toJS(c).b !== b) // false, cloned
    t.ok(mobx.toJS(c).b.x === b.x) // true, both 3

    t.ok(mobx.toJSlegacy(c).a !== a) // false, cloned as well
    t.ok(mobx.toJSlegacy(c).b !== b) // false, cloned
    t.ok(mobx.toJSlegacy(c).b.x === b.x) // true, both 3

what do you think @andykog @maxdeviant?

EDIT: fixed example above

[coveralls]

Coverage decreased (-0.3%) to 95.722% when pulling 6672288 on fix-566-toJS into 8471ffb on fix-571-no-instanceof.

[maxdeviant]

@mweststrate From what you described this sounds like it solves the problem I was seeing.

I think that is what most people would expect from this function (I would)

I would agree with this. In fact, that was how I thought toJS worked initially.

[andykog]

@mweststrate Nice! By the way, what do you think about having non-enumerable ObservableObject#toJS by analogy with ObservableArray#toJS?

[mweststrate]

yeah thought about that, but it is a bit nasty as such a method could
easily (accidentally) be overwritten by the user (or setting a prop with
name toJS would fail on him), both scenario's are a bit scary

Op ma 3 okt. 2016 17:20 schreef Andy Kogut [email protected]:

@mweststrate https://github.com/mweststrate Nice! By the way, what do
you think about having non-enumerable ObservableObject#toJS by analogy
with ObservableArray#toJS?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#589 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvGhLJZcwzd1BXXPO4hWVOlSIWvtF13ks5qwR0ggaJpZM4KJuLG
.

[andykog]

@mweststrate, I agree, but that could be useful for debugging. May be call it debugToJS so the purpose is more clear?

[mweststrate]

hmm for debugging we could simply introduce object.$mobx.toJS() :)

Op ma 3 okt. 2016 17:31 schreef Andy Kogut [email protected]:

@mweststrate https://github.com/mweststrate, I agree, but that could be
useful for debugging. May be call it debugToJS so the purpose is more
clear?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#589 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvGhJXe5d67FaPApXaNUPh9gj5FHQH_ks5qwR_GgaJpZM4KJuLG
.

[andykog]

@mweststrate, yeah, that should work :-)

[mweststrate]

@andykog mind opening a separate issue? or creating a PR ;-) ?

Op ma 3 okt. 2016 om 17:40 schreef Andy Kogut [email protected]:

@mweststrate https://github.com/mweststrate, yeah, that should work :-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#589 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvGhJnGC3I8u7-9geKTmdrRLydU53ufks5qwSHngaJpZM4KJuLG
.

[andykog]

I'll do a PR

[donaldpipowitch]

This broke our app (and it wasn't the first time with an update). It would be nice if this could be solved differently in the future:

• introduce a new function, instead of changing the old one and introducing a new function with old behaviour OR
• deprecate a function with a patch/minor release (only adds a deprecation log for non-production builds) and change the function to the new behaviour in a major release later

Please have no hauptversionsnummernerhöhungsangst :)

[mweststrate]

@donaldpipowitch you're right, I'm definitely scared of major number version bumps :'(. I'll gonna deal with that and join the anonymous-club-for-only-minor-version-bumpers

[donaldpipowitch]

No problem. Making frameworks is hard and you do a good job.

[capaj]

Making frameworks is hard

@donaldpipowitch that's why we're only making a library 😀

[ditiem]

As an example for the 2nd point:

if there is an observable object, deep inside an observable object, with some non observables in between, the deep observable object won't be seen by toJS and hence not converted. Is that a problem? (I think it is a rare case)

It may not be such a rare case. I ended up with an array of arrays and a computed getter like this:

@observable _rows = [ ] ;
@computed get rows ( ) { return this._rows.slice( 0 ) ; }

This is because some components of other libraries do rely on a is-array-test that requires the array to be truly an array (and it is out of my hand to change anything). For instance, this happens with propTypes in react.

now, toJS( my_class.rows ) fails to convert because the elements of rows are an observable but the first element of toJS is a (non-observable) array.

As you have flags as second argument, I think it will be a good idea to put a flag that simply transverse the object without looking if it is observable or not. Or maybe a flag to consider the arrays as you have one for the objects.