Immer: Idea: Ignore return value of producer

[mweststrate]

Currently the return value (if any) matters a lot, to be able to replace the current state. So there are quite some rules

1. Return undefined (or no return at all), will assume the draft is the state to be returned
2. Return any value (except draft or undefined), will replace the state with the returned value

This cause a few tricky cases:

• Because of 1, we cannot replace the current state with undefined, by simple returning undefined. For that reason, nothing has to be returned
• Because of 2, it is easy to accidentally replace the state, for example in produce({ x: 1}, draft => draft.x = 2) will produce 2 as the next state, not { x: 2 }. (Since assignment is statements return their right hand value, the result of the function is 2, not undefined. To avoid this, we have to write either draft => { draft.x = 2 } or draft => void draft.x = 2.

We could design the API differently, and always by default ignore the return value of a producer, regardless what the return value is. This fixes point 2, where we accidentally replace the state.

To be able to replace the state, we could have a dedicated internal indicating that we intend the replacement: return replace(3). This also solves 1), as undefined has no longer to be treated specially, and we can just return replace(undefined).

Note that we could even detect a replace call without return and warn the user.

Beyond that, we can possible warn about return a value, but not updating the draft (probably replace was intended)

So, in summary:

import { produce, replace } from "immer"

produce({ x: 1 }, draft => draft.x = 2) 
// returns { x: 2 }

produce({x : 1}, draft => replace(2))
// returns 2

produce({x : 1}, draft => replace(undefined))
// returns undefined

produce({ x: 1 }, draft => {
   return replace(3)
})
// returns 3

produce({ x: 1 }, draft => {
   draft.x = 2
   return 3
})
// returns { x: 2 }
// The '3' is ignored entirely (and we can't really warn about it)

Alternatively we could warn about non-undefined return values (fixing the last case) and forcing consumers to either use d => { ... } or d => void ... or d => replace(...) in producers.

_Edit: removed the warnings_

[aleclarson]

IMO: Warn when draft is mutated and _not_ returned. Then whenever a producer returns undefined, replace the draft with undefined.

produce(draft => {
  draft.a = 1
  return draft   // This line is required when you mutate `draft`
})

produce({}, draft => undefined) // => undefined

[aleclarson]

The replace function doesn't solve the second issue, which is the need for another import. I should be able to create producers in one module without importing immer, then import those producers in an immer-aware module that wraps them and re-exports.

[aleclarson]

I really like how the "return the draft" requirement discourages footgun one-liners:

// The footgun
produce({}, draft => draft.a = 2) // => 2

// When the draft must be returned, safe one-liners are more intuitive.
produce({}, draft => (draft.a = 2, draft))

[mweststrate]

@aleclarson do you mean in the current, or new situation? The proposal here is to never care about what is returned, so the above could also be written as () => draft.a = 2, or, with the more stricter proposal _needs_ to be written as () => { draft.a = 2 } / () => void (draft.a = 2)

[albertogasparin]

I've also being bitten a couple of times by:

produce({ x: 1 }, draft => draft.x = 2) // => 2

If you want to replace a state why not call produce with the new state

produce({ x: 1 }, { a: 2 });

As the function is kind of superfluous. If you do so, you can go with always ignoring the value returned by the function:
```js
produce({ x: 1 }, draft => draft.x = 2)
// returns { x: 2 }

[aleclarson]

@albertogasparin There's no point in using produce in your proposed produce(currState, nextState). Just do currState = nextState when you know the next state already.

[aleclarson]

Do you mean in the current, or new situation?

@mweststrate My proposal is separate from yours.

The proposal here is to never care about what is returned

My proposal is the opposite: "The return value is always used"

I think explicit returns are the easiest to reason about. And no need for a replace function.

[albertogasparin]

@aleclarson How do you avoid erroneous returns? As you pointed out, even if we say that the returned value is used as new state, arrow function still makes it very error prone:

// The footgun
produce({}, draft => draft.a = 2) // => 2

I like more the main proposal (ignoring the return), which will let immer care only about the changes made to draft.

@mweststrate Besides I don't understand why would someone to use immer to replace the state with undefined (or any other statically known value) inside a produce. As @aleclarson said "Just do currState = nextState". Am I missing something? 🤔

[aleclarson]

Besides I don't understand why would someone to use immer to replace the state with undefined (or any other statically known value) inside a produce. As @aleclarson said "Just do currState = nextState". Am I missing something? 🤔

Yes, producers may use conditional logic to determine if no changes, some changes, or a replacement is appropriate.

---

Is optimizing for one-liner producers really worth it? Are people using one-liners more often than they are replacing the base state? I'm really not a fan of adding a replace function. Maybe there's a better way..

edit: I think it's simpler for developers to learn "the return value of a producer is always used, just like in a reducer" instead of "the return value is only used if you wrap it with a replace call".

[mweststrate]

As I've said in #246, I'm highly against ignoring the return value by default. The current behavior is much better, and I think the void trick should be discouraged in the docs, since it's just a footgun for no real gain. If anything, the return value should always be used so the mental model is closer to a traditional reducer. In the end, there's really not a strong enough justification for changing the current behavior at all.

1. Returning the value is unnatural, the normal case of using a producer is to modify that thing that you are passing in to it. Needing to write draft => draft.x++, draft (or in other forms) is imho counter-intuitive. In that case I prefer the current 'footgun' where you shouldn't forget void, but in that case at least a normal {..} function body produces the required result
2. Producing an entire new value, not based on the value you started with, is the exceptional behavior (don't think I saw it outside Redux reducers). That makes perfect sense; the goal of a producer is to produce a new value _based on_ the current value. If the goal is to replace a value, produce is in principle not needed in the first place! The only reason why we have such a case, is that in reducers it is quite common to have different code paths that either "update" the current state, or entirely replace it with something else.

To summarize, the options are:

Updating a value (the common case)

1. draft => void draft.counter++ or draft => { draft.counter++ } (as is)
2. draft => draft.counter++ or draft => { draft.counter++ } (the proposal)
3. draft => draft.counter++, draft or draft => { draft.counter++; return draft } (proposal @aleclarson )

Replacing a value (the non-common case)

1. draft => ({ counter: 0}) or draft => { return { counter: 0 } } (as-is)
2. draft => replace({ counter: 0}) or draft => { return replace({ counter: 0 } }) (the proposal, using replace utility)
3. draft => ({ counter: 0}) or draft => { return { counter: 0 } } (as-is) (proposal @aleclarson )

Replacing state with undefined (the really exceptional case)

1. draft => nothing (as-is, using nothing token)
2. draft => replace(undefined) (the proposal, no longer an exceptional case)
3. draft => undefined

[aleclarson]

My vote is to keep the current behavior.

Note: The replace function in option 2 should only be required if the draft has been modified.

import {produce, replace} from 'immer'

produce({ count: 0 }, draft => draft.count++)
// => { count: 1 }

produce({ foo: true }, draft => ({ foo: false }))
// => { foo: false }

produce({ a: 1 }, draft => {
  draft.a = 2
  return replace({}) // Throw an error if `replace` is not used
}) // => {}

Also, there could be a special case for original so you can do:

produce({ a: 1 }, draft => {
  doSomeMutations(draft)
  return original(draft) // Rollback to original state
}) // => { a: 1 }

[mweststrate]

My vote is to keep the current behavior.

Yeah, that is something I can live by as well :)

Note: The replace function in option 2 should only be necessary if the draft has been modified.

I think that will be too error prone / subtle. The result of produce will depend on unclear ways on the code paths taken in the produce function itself. (we could otherwise apply this already to the current implementation; only inspect return value if draft not modified).

[mweststrate]

Made a poll: https://twitter.com/mweststrate/status/1092341986374438912

[alitaheri]

I agree with @aleclarson on this. I also agree that the void trick should be removed from the docs.

But I disagree that we should always return the draft. because that would confuse someone looking at it for the fist time.

right now, what we have is perfect. just remove void trick. I have the following justifications:

1. We shouldn't extend the api beyond the scope for no real gain
2. This library doesn't represent traditional reducers, so it shouldn't be confused with them, we're recording mutations and building a new state from it, not reducing state with mutation to get a new state.
3. we should encourage readable code. the void (...) pattern is not readable for someone who doesn't know why it's there. because they should know what void does, keep in mind that => expr returns the evaluated value and why it's there in the first place (returned values the callback mean something special)
4. We need a way to support changing the end value in a composeable way! replace(value) specializes this functionality. so a function from another library, or a shared function must be aware of immer. which is a framework smell. I don't like the nothing solution either, but that's on javascript not immer.

[alitaheri]

Also, we should NOT warn for non-undefined returned values:

// this will return 1 and warn, we don't want that. assignment expressions return the assignee, remember
const newState = produce(state, draft => draft.a = 1);

In any case, I have another proposition.

Just another similar proposal:

import produce, { replaced } from 'immer';

// -> {blah: 1}
const newState = produce(state, draft => {
  return replaced({blah: 1});
});

// -> {blah: 1}
const newState = produce(state, draft => replaced({blah: 1}));

// -> undefined (remove nothing heler)
const newState = produce(state, draft => replaced());

// -> null
const newState = produce(state, draft => replaced(null));

// -> 1
const newState = produce(state, draft => replaced(1));

// {a: 1} , returned value is ignored
const newState = produce(state, draft => draft.a = 1);

Slightly more readable that calling replace. and it's somewhat composeable. because we can add another function for it:

import produce, { replaced, isReplaced } from 'immer';


const newState = produce(state, draft => {
  const val = functionWorkingWithDraft(draft);

  if (isReplaced(val)) {
    return val;
  }

  // continue
});

I'm not very sure about this, but at the very least, it's a bit more readable

[mweststrate]

This library doesn't represent traditional reducers, so it shouldn't be confused with them, we're recording mutations and building a new state from it, not reducing state with mutation to get a new state.

That is actually the reasoning behind this proposal. Conceptually, return values from recipes don't make any sense / are meaningless; they are to mutate the draft. The only reason why evaluating the return value was added, later on (in 1.1, commit), was to support the typical redux case of returning new stuff. Which in turn introduced the work-arounds of void and nothing.

So by going back to the original behavior, immer / producers themselves get cleaner, except for the exceptional redux case, where it is made more explicit that _intentionally_ a new state is returned, by using replace.

[mweststrate]

@alitaheri ehh.... I think, beyond the name, that is exactly the current proposal?

[alitaheri]

@mweststrate no, it's not. it's a bit different

I'm saying any return value must be ignored EXCEPT when it's the result of replaced. because we want the user to have control outside of functions that might call replace.

produce({ x: 1 }, draft => {
   replace(3)
})

vs

produce({ x: 1 }, draft => {
   return replaced(3)
})

this way, if a nested function returned replaced result, the user can ignore it, or take it into account.

having a function that changes the behavior of a whole subtree is an anti pattern, a framework smell, and we hard to compose.

[mweststrate]

@alitaheri ok, lol, what you are saying was intended to be the proposal. Will update the original post, the example code is broken :').

Edit: well, the warnings are different indeed then original proposed. But I agree, and removed them from the proposal.

[mweststrate]

Poll results (102 votes):

08% keep as is!
52% awesome idea!
40% results

Seems worth pursuing in a MR

[aleclarson]

/cc @knpwrs @jineshshah36 @markerikson

Your opinions would be valuable!

[jineshshah36]

I would definitely agree with ignoring the return value. However, I would be curious to know what the return type of replace is as well as the type of nothing. To make this design the most type safe, I would consider doing the following:

const nothing: unique symbol = Symbol()
const replaced: unique symbol = Symbol()
const replace = (value: any): typeof replaced => {
  // do replacement with value
  return replaced
}

// enforce the return type of produce is the following:
type Return = typeof nothing | typeof replaced | void

// JS users could still do produce({ count: 0 }, draft => draft.count++) with no issue,
// but typescript would error. However, I think the TS users would appreciate the
// increased safety over the small inconvenience.

[markerikson]

Coming from the Redux point of view, I would definitely be _against_ ignoring the return value, for two reasons:

• There's still plenty of idiomatic use cases for wanting to generate a new value using existing immutable methods, like removing an item using return state.filter(item => item.id !== someId)
• I think most Redux users of Immer will be doing so via something that wraps produce() and calls it behind the scenes, rather than explicitly calling produce() themselves inside their own reducer. Specifically in the case of Redux Starter Kit, we do this:

export function createReducer<S = any, A extends Action = AnyAction>(
  initialState: S,
  actionsMap: CaseReducersMapObject<S, A>
): Reducer<S> {
return function(state = initialState, action): S {
return createNextState(state, (draft: Draft<S>) => {
      const caseReducer = actionsMap[action.type]
      return caseReducer ? caseReducer(draft, action as A) : undefined
    })
  }
}

From the end user's point of view, there's no visible evidence that Immer is being used:

const todosReducer = createReducer([], {
    "ADD_TODO" : (state, action) => {
        // "mutate" the array by calling push()
        state.push(action.payload);
    },
    "TOGGLE_TODO" : (state, action) => {
        const todo = state[action.payload.index];
        // "mutate" the object by overwriting a field
        todo.completed = !todo.completed;
    },
    "REMOVE_TODO" : (state, action) => {
        // Can still return an immutably-updated value if we want to
        return state.filter( (todo, i) => i !== action.payload.index)
    }
}

Removing the ability to return a new value would cause us issues.

We do have some warnings in our RSK docs about the fact that we're using Immer, that it's what makes the "mutating" possible, and the caveats around both mutating and returning a new value, so the abstraction is admittedly a bit leaky to begin with. But, I'd prefer not to have to force users to explicitly rely on Immer's API for replacement behavior.

Then again, I suppose we could make tweak our createReducer function to check for a return value from the reducer and call replace().

[jineshshah36]

@markerikson You could call replace() in prod but also warn when in dev so that you could gradually migrate

[knpwrs]

/cc @knpwrs [and others]

I changed knpwrs/redux-ts-utils to use createDraft/finishDraft so I don't believe this proposed change would affect me.

[alitaheri]

@markerikson Raises a good point. I have an idea. We can't just warn by default since cases like this:

return produce(state, draft => draft.a = 2);

will warn for no good reason.

However, we can add an opt-in options: calling enableReturnValueUsageWarning() will enable the error. and we can document it in the migration section. opt-in to avoid confusion in the aforementioned case.

I'm still not 100% bought in the idea, but the community agrees that we should go down this road, so let's at least enable gradual migration paths.

P.S. the warning must use console.error to show the stack trace, makes it a lot easier to track down usages.

[alitaheri]

@jineshshah36 I think we could overload the produce function. if it matches (draft => () => Replaced<T>) then return T. Otherwise, State. I'm not sure, but when it's done I could play with it a bit.

[jineshshah36]

@alitaheri the point of typing Return as typeof nothing | typeof replaced | void is to ensure that users only return one of those three rather than just anything. I'm not sure an overload could enforce that

[alitaheri]

@jineshshah36 oh, we shouldn't enforce that. because of this:

return produce(state, draft => draft.a = 2);

the assignment expression evaluates to 2 and the arrow function returns it. so if we type it in a way that disallows return type of not Replaced, we will be disallowing such usages (which is one of the reasons for this idea)

[mweststrate]

Typewise it is not very hard to make that work produce<D, R>(state: D, (draft: D) => R): R extends Replace<infer X> ? X : D, where replace is defined as replace<T>(value: T>): Replace<T>

[mweststrate]

@markerikson thanks for the clarification, that is really insightful! I think the abstraction leaks in any case indeed, but I can imagine that you don't want to explicitly use an api of immer for replacement. (Let's throw replacement values wrapped in an exception, lol :P)

[mweststrate]

Ok, closing this for now. So far everybody seems to be getting along with void or { } quite well, and the abstraction is going to leak _somewhere_ when using produce inside external abstractions like RSK:

Either:

• void is needed to avoid accidental replacements
• replace needs to be introduced to support replacements

I think in hind-sight, replacing the returned state should not have been supported in the first place (it's not the point Immer, just made currying for reducers more easy), which leaves it to the wrapping abstraction to deal with those cases (need replace? don't call produce in the first place...). But I don't think it is worth the effort atm to enforce such a breaking change on the community :)

[Noitidart]

In react I am doing:

this.setState(produce(draft => {

});

But then as I am doing changes to the draft, I realize I want to cancel the setState by returning null. Is it possible to do this somehow? As returning null in setState cancels.

I also have this problem with hooks:

            const [ pastes, setPastes ] = useState([]);
            setPastes(produce(draftPastes => {
                const draftPaste = draftPastes.find(paste => paste.id === pasteId);
                if (!draftPaste) return null;
                draftPaste.comments.push(comment);
            }));

[mweststrate]

Afaik that should work OOTB. Code you create a sandbox demonstrating the
issue?

Note that it is not possible to return undefined from a producer, in that
case, one should return NOTHING (can be imported from immer)

On Wed, May 8, 2019 at 4:22 AM Noitidart [e-mail] wrote:

In react I am doing:

this.setState(produce(draft => {

});

But then as I am doing changes to the draft, I realize I want to cancel
the setState by returning null. Is it possible to do this somehow? As
returning null in setState cancels.

I also have this problem with hooks:

        const [ pastes, setPastes ] = useState([]);
        setPastes(produce(draftPastes => {
            const draftPaste = draftPastes.find(paste => paste.id === pasteId);
            if (!draftPaste) return null;
            draftPaste.comments.push(comment);
        }));

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/immerjs/immer/issues/246#issuecomment-490321845, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAN4NBE26QAXC6FF6IAR6CDPUI2MXANCNFSM4GCZPBDA
.