Diff immutable stores not working properly

[ingro]

Hello! I'm trying to use this monitor with some Immutable stores. The changes are caught just fine but they are not displayed correctly, like this:

timeline._root.entries.0.1: [] [3,50,1,4,52,51] 
timeline._root.entries.1.1: 0 1 
timeline._root.entries.2.1: 0 1 

Since the default LogMonitor display Immutable stores correctly, do you think that should be possible to implement the same algorithm here too?

[iktl]

Can you confirm if this is resolved by #15 which I just merged into 2.0.2?

[ingro]

Thanks for your extremely fast reply! :)

I dunno if I'm doing something wrong with Immutable (I'm just using it from few days) but now I got this result:

ACTION
{"type":"MERGE_TIMELINE","item":{"ids":[2,4,51,1,52,50],"turn":1,"round":1}}

STORE MUTATIONS
timeline.ids: undefined  
timeline.ids: undefined  
timeline.ids: undefined  
timeline.ids: undefined  
timeline.ids: undefined  
timeline.ids: undefined  
timeline.turn: 0 1 
timeline.round: 0 1 

As you can see ids shoul be equal to an array (like it was before), but now I got undefined for each item, I just updated the diff monitor, the application code should be the same.

[iktl]

This might take a little longer to take a look at, I'll have to set up an immutable test example and mess around over the weekend I'm afraid. If you want to poke around the code for the diff-monitor and beat me to it let me know, but I'll get back to you.

[ingro]

Thanks for your reply, I can't promise anything but I'll try to mess around
with the diff-monitor code, even if my experience with Immutable is very
limited! I'll let you know!

On Fri, 25 Sep 2015 18:29 Isaac Luxford notifications@github.com wrote:

This might take a little longer to take a look at, I'll have to set up an
immutable test example and mess around over the weekend I'm afraid. If you
want to poke around the code for the diff-monitor and beat me to it let me
know, but I'll get back to you.

—
Reply to this email directly or view it on GitHub
#16 (comment)
.

[ingro]

@iktl seems like using toObject() instead of toJS() in the diff-monitor code produced the desired result.

So it should become:

newState = Immutable.Map(this.props.computedStates[index].state).toObject();
oldState = Immutable.Map(this.props.computedStates[index - 1].state).toObject();

[theSmaw]

@ingro are you able to show us the reducer that is operating on your store?

.toObject() doesn't quite work when working with a store that is a plain JavaScript object at the top-level, but contains immutable structures within it.

e.g, my store looks something like

{
    app: Immutable.Map(),
    something: Immutable.Map()
}

Converting this with Immutable.toObject() means that the children stay as immutable objects, rather than becoming plain JavaScript objects that can be properly diffed.

Using .toJS() 'should' unwrap immutables all the way down the tree.

https://facebook.github.io/immutable-js/#converts-back-to-raw-javascript-objects-

@iktl Do you mind if I start putting some unit tests against this project to make sure edge cases are being properly covered for areas like this? Do you have a preference (Mocha, Jasmine, etc?). Or would you rather own this yourself?

[theSmaw]

reduxjs/redux-devtools#79 - looks a more comprehensive solution and allows for iterables other than just Immutable.

[iktl]

@theSmaw mocha or jasmine are fine by me. Any contribution to testing would be great, I don't have a lot of capacity right now. Thanks in advance!

[ingro]

Thanks for the informations @theSmaw , here's my reducer:

import { Map } from 'immutable';

const DEFAULT_STATE = Map({
    ids: [],
    turn: 0,
    round: 0
});

function merge(state, item) {
    state = state.merge(item);

    return state;
}

export default function(state = DEFAULT_STATE, action) {
    switch (action.type) {
        case 'MERGE_TIMELINE':
            return merge(state, action.item);
    }

    return state;
}

As I said it's pretty basic stuff, I'm a newbie with Immutable so maybe I'm missing some basic problem.

[theSmaw]

Should be fixed by #18

[ingro]

Yep, now it's looking good, thanks @theSmaw !