leoasis / redux-immutable-state-invariant

Redux middleware that detects mutations between and outside redux dispatches. For development use only.
MIT License
937 stars 37 forks source link

Structural Sharing #16

Closed santiagopuentep closed 8 years ago

santiagopuentep commented 8 years ago

In my reducer I´m copying this object...

const myObject = {
    text: "test",
    children: ["one", "two"]
}

...doing this:

const myCopy = {
    ...myObject
}

The middleware throws an error. Shouldn't this work? I know I'm reusing the array but this would be doning "structural sharing". I'm not mutating the original object and I'm wasting less memory.

Is this correct or am I understanding something wrong?

In this case I have to copy like this to fix the error:

const myCopy = {
    ...myObject,
    children: [...myObject.children]
}

Thanks!

leoasis commented 8 years ago

Hi, thanks for trying the library!

Could you provide a more complete example that causes this to fail? The code you provided on its own is not enough info because I don't know how you're using myCopy later on, or what are you actually doing with the state. If you can't copy your current use case because it's not something you're allowed to share, then try to come up with a minimal example so I can understand what you're trying to do and if it's actually an issue with the library or not.

Thanks!

santiagopuentep commented 8 years ago

I´m simply doing the change to the state inside a reducer.

With this state:

{
    item: {
        text: "test",
        children: ["one", "two"]
    }
}

Using this reducer:

const changeTextReducer = (state, action) {
    if (action.type === "CHANGE_TEXT") {        
        return {
            item: {
                ...state.item
                text: action.text
            }
        }
    }
}

Throws an error because the children array was copied as a reference. This shouldn´t throw an error because the unchanged parts of the state should be able to be reused.

Here´s a video explaining what I mean. https://youtu.be/I7IdS-PbEgI?t=5m6s

In order to dodge the error I have to change the reducer to:

const changeTextReducer = (state, action) {
    if (action.type === "CHANGE_TEXT") {        
        return {
            item: {
                text: action.text
                children: [...state.item.children]
            }
        }
    }
}

But I'm duplicating the children array that I didn´t change.

leoasis commented 8 years ago

As you say, the first reducer code is correctly doing an immutable operation, and should not fail with this library. And I just tested that, and effectively it doesn't fail. So maybe there is something else being mutated elsewhere, or the problem is something else.

Would you try and create a sample repo or js fiddle with the minimal code that causes this issue? Or maybe even try and write a test (check examples in https://github.com/leoasis/redux-immutable-state-invariant/blob/master/test/trackForMutations.spec.js) that outlines the problem?

Edit: Actually it looks like there is a test for exactly your example, here: https://github.com/leoasis/redux-immutable-state-invariant/blob/master/test/trackForMutations.spec.js#L194-L205. Unless there's something I'm not seeing?

You definitely shouldn't need to do the array shallow copying. The fact that you do that makes me wonder if you maybe mutate that children somewhere else, even outside the reducer, and that may be causing the issue.

santiagopuentep commented 8 years ago

I tried reproducing the issue but I couldn´t. That specific case is long gone in my project. I'm reactivating this package. I'll let you know if I have any issues again.

Thanks!

leoasis commented 8 years ago

@santiagopuentep thanks! Send an issue if you come back to it