Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed behavior between 1.6.0 and 1.5.0 #1327

Closed
sag-tobias-frey opened this issue Jul 20, 2021 · 13 comments
Closed

Changed behavior between 1.6.0 and 1.5.0 #1327

sag-tobias-frey opened this issue Jul 20, 2021 · 13 comments

Comments

@sag-tobias-frey
Copy link

sag-tobias-frey commented Jul 20, 2021

The following code prints false, true true in v1.5.0 and false, false false in v1.6.0. Seems to be related to some createSelector changes.

const {combineReducers, createSelector, createSlice, createStore} = require('@reduxjs/toolkit');

const initialState = {
    container: {
        template: {

        }
    }
};

const testSlice = createSlice({
    name: 'test',
    initialState,
    reducers: {
        setBlock(state, action) {
            const calculation = state.container.template;
            calculation.block = action.payload;
        }
    }
});

const store = createStore(combineReducers({
    test: testSlice.reducer
}));

let oldState = store.getState();
function compare(newState) {
    console.log(oldState === newState);
    oldState = newState;
}

const selector = createSelector((state) => state.test.container, (state) => state.template);

store.dispatch(testSlice.actions.setBlock());
compare(selector(store.getState()));
store.dispatch(testSlice.actions.setBlock());
compare(selector(store.getState()));
store.dispatch(testSlice.actions.setBlock());
compare(selector(store.getState()));
@phryneas
Copy link
Member

phryneas commented Jul 20, 2021

createSelector was not changed, we added only a new api, createDraftSafeSelector.

What did indeed change, was that we are freezing the initial state passed into createReducer and createSlice now, which in fact does create an new object reference directly after store creation.

So where previously, an action targeted at an empty reducer case might have triggered that "first freezing" and as such an involuntary rerender, that state is now frozen from the beginning, avoiding that rerender.

I would consider that a bugfix, as the behaviour from before was definitely not correct.

@sag-tobias-frey
Copy link
Author

I can see that the old behavior is wrong. However, the new behavior is producing more rerenders for me because previously starting with the second action, the state did not change anymore. While with the new behavior, the state is always changing.

I noticed one endless loop in my code base after updating to v1.6.0 because of this change. Although, the code triggering this effect was definitely wrong.

@phryneas
Copy link
Member

Apart from that "initial freeze" nothing on our side changed and nothing could explain that changed behaviour you are experiencing there. Could this have something to do with the change to immer 9?

@markerikson
Copy link
Collaborator

Freezing doesn't, as far as I know, return a new reference. It just marks the existing reference as being a frozen object.

The example is confusing, because the reducer references action.payload and yet the dispatches are setBlock() with no payload.

Can you put up a CodeSandbox that actually demonstrates what you're talking about?

@phryneas
Copy link
Member

I think the problem seems to be that assigning a value (here undefined) to an object property that already has that exact value results in a new state reference.

@sag-tobias-frey
Copy link
Author

sag-tobias-frey commented Jul 20, 2021

Sure, the CodeSandbox is exactly the same and can be found here:

-> https://codesandbox.io/s/cold-cookies-ztywn

I have updated the action to explicitly use undefined instead of leaving out the parameter which will result in the same problem. The sandbox will return false, true, true in the old version; changing to v1.6.0 will print false, false, false.

@markerikson
Copy link
Collaborator

I really can't think of any changes we would have made that would relate to this.

Can you try putting together a similar demo that only uses Immer, and compare behavior with Immer v8 vs v9?

@sag-tobias-frey
Copy link
Author

Yesterday, I was not able to reproduce the issue with only immer. However; I got the same example now showing the same effect with immer only.

-> https://codesandbox.io/s/elegant-http-ozx9o

@phryneas
Copy link
Member

I took your test and rewrote the "test" part of it to be more legible. Nothing unexpected happens there. I assume it is an error of the test - take a look here:

https://codesandbox.io/s/fancy-cdn-jj2pd?file=/src/index.js

@sag-tobias-frey
Copy link
Author

@phryneas Changing @reduxjs/toolkit to v1.6.0 shows the errors again; that's interesting.

@phryneas
Copy link
Member

phryneas commented Jul 21, 2021

I see that now, but it doesn't seem to appear in 1.6.1.
So either way, this seems to be fixed in the latest patch release. Probably related in some bizarre way to #1305 - which is the only PR in that release that even touched the folder related files are in.

@sag-tobias-frey
Copy link
Author

sag-tobias-frey commented Jul 21, 2021

Thanks for investigating the issue. I also rechecked with only immer and the issue seems to be fixed with 9.0.5; version before that show the issue. Seems to be related to immerjs/immer#807

@phryneas
Copy link
Member

Okay, then it's probably that.
Given that we can't do anything about it and it's fixes I'm gonna close this here - thanks for the report :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants