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

Nested Produce: Changes to inner draft not reflected in outer draft #694

Closed
3 tasks done
RoccoC opened this issue Oct 30, 2020 · 7 comments
Closed
3 tasks done

Nested Produce: Changes to inner draft not reflected in outer draft #694

RoccoC opened this issue Oct 30, 2020 · 7 comments
Labels

Comments

@RoccoC
Copy link

RoccoC commented Oct 30, 2020

🐛 Bug Report

Nested produce calls are not working as expected; changes made to the "inner" draft do not reflect on the "outer" draft.

My use case is calling a sub-reducer from a main reducer, where the main reducer makes some mutations to its draft, and then passes the draft as a base state to a sub-reducer where further changes to the draft will be made. For example:

const innerReducer = produce((draft) => {
  draft.y++;
});

const outerReducer = produce((draft) => {
  draft.x++;
  // now delegate some work to a sub-reducer
  innerReducer(draft);
});

const newState = outerReducer({ x: 0, y: 0 });
// newState is now equal to { x: 1, y: 0 }, but I expect { x: 1, y: 1 }

In this example, my expectation is that newState contains the changes made by both the outerReducer and the innerReducer, but the draft changes made in innerReducer are not reflected.

Link to repro

https://codesandbox.io/s/immer-sandbox-forked-mqjns

or

https://codesandbox.io/s/immer-sandbox-forked-6jptr

To Reproduce

Pass a draft state as base state to produce, e.g. by nesting one or more produce calls.

You can also paste this test case into an existing test suite:

it('allows for nested produce calls', () => {
  const innerReducer = produce((draft) => {
    draft.foo = 'bar';
  });

  const outerReducer = produce((draft) => {
    innerReducer(draft);
  });

  const state = { foo: 'foo' };

  expect(innerReducer(state)).toEqual({ foo: 'bar' });
  expect(outerReducer(state)).toEqual({ foo: 'bar' }); // <-- this fails :(
});

Observed behavior

Changes made to inner draft object are not reflected in the outer draft object.

Expected behavior

Reading the discussions in #433 and #533, I would expect that changes made to nested draft objects should be reflected in the top-most draft object.

Environment

  • Immer version: 7.0.14
  • I filed this report against the latest version of Immer
  • Occurs with setUseProxies(true)
  • Occurs with setUseProxies(false) (ES5 only)
@RoccoC
Copy link
Author

RoccoC commented Oct 30, 2020

So the more I think about this, the more I convince myself that this behavior is working as designed.

The interesting thing about Immer is that the baseState will be untouched, but the nextState will reflect all changes made to draftState.

Since the main idea behind produce is to not mutate the base state, this would mean that I should expect that the outer draft I am passing to the inner produce call as the base state should not be mutated (and it's not). :)

In my example I am essentially discarding the results of the inner reducer, when I should instead merge that result with the outer reducer's draft object, e.g.:

const innerReducer = produce((draft) => {
  draft.y++;
});

const outerReducer = produce((draft) => {
  draft.x++;
  // now delegate some work to a sub-reducer, merge the new state with my draft
  Object.assign(draft, innerReducer(draft));
});

const newState = outerReducer({ x: 0, y: 0 });
// newState is now equal to { x: 1, y: 1 }

That being said, I think this actually conflicts with some of the past discussions around this topic (#377, #433, #533), specifically #533 (comment). I'd like to leave this open in hopes that one of the project maintainers can confirm the expected behavior in this scenario. This could also be a good opportunity to add to the docs to prevent any confusion in the future (I'll be happy to submit a PR once the expected behavior is confirmed).

@mweststrate
Copy link
Collaborator

Your analysis sounds about right, note that in your example just innerReducer(draft); would have done the trick. Feel free to open a PR to explain the semantics in more detail :)

@RoccoC
Copy link
Author

RoccoC commented Nov 3, 2020

Thanks for the reply!

note that in your example just innerReducer(draft); would have done the trick.

Are you saying that there is no need to merge the inner and outer drafts with something like Object.assign(), and that this should work?

const innerReducer = produce((draft) => {
  draft.y++;
});

const outerReducer = produce((draft) => {
  draft.x++;
  innerReducer(draft);
});

const newState = outerReducer({ x: 0, y: 0 });

...because it doesn't. :) Without Object.assign(), any mutations made to the draft in innerReducer() are lost, and in this example, newState becomes { x: 1, y: 0 }.

@mweststrate
Copy link
Collaborator

Sorry, I meant to type return innerReducer(draft). If you return anything else than the draft or nothing from a producer, that will become the returned stated.

@RoccoC
Copy link
Author

RoccoC commented Nov 3, 2020

Got it! Let me put together a PR to clarify this scenario in the docs so that there's no confusion going forward. Thanks again!

@aleclarson
Copy link
Member

🎉 This issue has been resolved in version 7.0.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jesseko
Copy link

jesseko commented Dec 31, 2020

For anyone trying to navigate use of nested calls to produce, I made a codepen demo of things that do and don't work
uses Immer 8.0.0
https://codepen.io/jesseko/pen/MWjQomG

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

No branches or pull requests

4 participants