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

Why immer inside a starter kit? #5

Closed
ematipico opened this issue Mar 6, 2018 · 12 comments
Closed

Why immer inside a starter kit? #5

ematipico opened this issue Mar 6, 2018 · 12 comments

Comments

@ematipico
Copy link

ematipico commented Mar 6, 2018

This is a genuine question. I have been using redux for long time and one of the pillars is having pure reducers. And I also know what immer does.

As this is a starter kit, so a bulk of code for people that want start from scratch (newbies and pros), what are the reasons for putting immer inside the starter package?

A new comer that is trying to learn redux cold have difficulties or be confused.

Thanks in advance for the feedback!

@nickmccurdy
Copy link
Collaborator

nickmccurdy commented Mar 6, 2018

Good question. One of the reasons we decided to include immer is to address the issue of users having accidental store mutations because they don't know that Redux requires immutable updates, or they don't know how to do immutable updates. We discussed this in more detail in reduxjs/redux#2858 and reduxjs/redux#2859 (comment).

I was honestly against immer at first for similar reasons to yours, but Mark convinced me this is a good idea because it safeguards impure reducers to force them to be pure while still allowing reducers to be pure when desired (so immer's addition should be totally back compatible, there's probably some performance overhead at least). My only concern is that this could teach users starting with only the starter kit that Redux allows immutable updates. I don't think there's a better way to solve this because store mutation checks will not be added to Redux's core (see reduxjs/redux#2858). I think immer is a good fit here, but let us know if you have any ideas for alternatives or improvements to this approach.

@markerikson
Copy link
Collaborator

Yeah, my one concern with using Immer is that someone who's not familiar with it might look at reducers and think "oh sure, I can write state[5].completed = true in any Redux app and it's fine".

But, the overall goal of the package is to simplify app setup and cut down on the overall amount of code users need to write. One of the biggest pain points with Redux is having to write immutable update logic by hand, and Immer is by far the best solution I've seen to that.

@nickmccurdy
Copy link
Collaborator

I'm also concerned about that. I think when this package is ready we should update the Redux docs to only show how to use Redux with the starter, that way it would be unusual or even "advanced" for a developer to use Redux without the starter kit, similar to how CRA is recommended in React's docs. Then there could be an advanced "usage without starter kit" section that discusses immutable updates in more detail. In other words this package would have the recommended usage of Redux, and Redux core would be a more advanced lower level abstraction for edge cases.

@ematipico
Copy link
Author

Yeah I admit that using manipulation inside a reducer is a big win especially for people that don't know how to create a pure function.

I wonder if it would be possible to expose a second function called createPureReducer which won't use immer under the hoods?

Imagine there's a senior that needs work on the performance and they know how to use redux, having immer in the way could create some overhead that they don't need. What do you guys think?

@markerikson
Copy link
Collaborator

Immer works by wrapping your state in a Proxy, but it also allows you to actually return a new state value directly as well.

Here's the current implementation of createReducer:

import createNextState from "immer";

export function createReducer(initialState, actionsMap) {
    return function(state = initialState, action) {
        const {type, payload} = action;

        return createNextState(state, draft => {
            const caseReducer = actionsMap[type];

            if(caseReducer) {
                return caseReducer(draft, payload);
            }

            return draft;
        });
    }
}

This is admittedly the first time I've ever used Immer, but my understanding is that if I explicitly return a value from a case reducer, Immer will use that as the result. Otherwise, if you don't return anything, Immer will check to see if the proxy was modified, and immutably generate a new value based on that.

So, I'm pretty sure that both of these will work correctly with createReducer as written:

function toggleTodoHandWritten(state, payload) {
    const {index} = payload;

    return state.map( (todo, i) => {
        if(i !== index) return todo;
        return {...todo, completed : !todo.completed};
    });
}

function toggleTodoImmer(state, payload) {
    const {index} = payload;

    const todo = state[index];
    // Can directly modify the todo object
    todo.completed = !todo.completed;
}

@ematipico
Copy link
Author

If it's possible to use both approaches, that's amazing!
Maybe it would be possible to pinpoint this example in the README, so people will know that they can use both ways without problems :)

Thanks for the clarifications!

@nickmccurdy
Copy link
Collaborator

nickmccurdy commented Mar 6, 2018

It would be great to add an immutable update example to createReducer.

@markerikson
Copy link
Collaborator

Sure, PR it :)

@Gabri3l
Copy link
Contributor

Gabri3l commented Mar 7, 2018

PR ready! #13

@nickmccurdy
Copy link
Collaborator

nickmccurdy commented Mar 7, 2018

#13 is merged. This should be enough to help new users understand using reducers with and without immer, but let us know if you have other ideas for improvements. I'm personally against adding a config option to disable it unless we can find a case where wrapping a pure reducer in immer is problematic.

@rabidpug
Copy link

rabidpug commented Mar 9, 2018

Would it make sense to rename the "state" parameter in the docs to "nextState" to make it clear you aren't actually mutating the current state, but mutating the future state?
eg

function addTodo(nextState, action) {
    const {newTodo} = action.payload;

    nextState.push({...newTodo, completed : false});
}

@nickmccurdy
Copy link
Collaborator

While I agree that nextState or draft is more technically accurate, I feel like the way immer works that's just an implementaiton detail that users shouldn't need to worry about. I'd rather keep it simple using concepts a Redux user is likely to already know like state to reduce confusion.

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

5 participants