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

Looking for feedback / testimonials #26

Closed
mweststrate opened this issue Jan 3, 2018 · 17 comments
Closed

Looking for feedback / testimonials #26

mweststrate opened this issue Jan 3, 2018 · 17 comments

Comments

@mweststrate
Copy link
Collaborator

Hi all!

I'm looking for feedback. If you tried / are trying this package, how do you experience it? Does it deliver what it promises? Does it help reduce amount / complexity of code? Is there anything you are missing or that could be improved in this package or the docs?

Any kind of feedback is appreciated, thanks!

@AriaFallah
Copy link

AriaFallah commented Jan 4, 2018

I really like it and it helps a lot. Don't have much else to say about that. Immutability is a lot of boilerplate, and this helps get rid of that boilerplate in a very simple way.

Personally, my biggest issue with it is that it's not very performant. I get that one shouldn't worry about performance until it's an actual issue, premature optimization is bad, and all the other tropes about performance; however, even given all of that, I'm not sure if being 5x-10x slower (by your benchmark) than the traditional handcrafted reducer it replaces is acceptable in the long term. I know you said the code is unoptimized though, and that proxies on v8 are always being improved so hopefully it won't be an issue when immer hits 1.0.

A smaller issue, which I'm not sure is solvable short of just saying "pay more attention," is that it can see myself forgetting to wrap mutation in immer, and getting actual mutation when I wanted immutability.

Otherwise, thanks for making this. It's super cool. As an alternative, I'm a big fan of https://github.com/substantial/updeep

@benbraou
Copy link
Collaborator

benbraou commented Jan 4, 2018

Great job for this library :)
I have started to use immer in a personal project.
As a library user, what I like most is amount of boilerplate it saves.
As a developer, I like the implementation details (use of proxy)

Documentation improvement: it would be nice to update the reducer example with REMOVE_TODO case. The goal is to show the usage of splice as opposed to filter. More generally, providing mutable operations that are 'equivalent' to the immutable ones would be great for newcomers.

It seems to me that the usage of immer depends on the flavor (functional oriented vs imperative) of the project. Personally, I have a project using almost only pure functions with no side effects. It goes with the philosophy with the project to avoid mutations (even on a draft state).
Here is a simple example:

const filterById = (id: number) => reject(compose(equals(id), prop('id')));

filterById is then used within the application's reducers.

@hex13
Copy link

hex13 commented Jan 4, 2018

I think good idea would be middleware for Redux which would automatically call transform

function someReducer(state, action) {
    state.foo = 123;
}

this way it would be even less verbose - no need for manually invoking function.

But I think two syntaxes should be supported. If somebody would refactor existing code, there will be both "mutable" and classic style reducers:

function someReducer(state, action) {
    return Object.assign({}, {value: action.value});
}

So it may detect if reducer returned an object (classic style) or not returned anything (so transforms).

Maybe even immer function should accept both returning syntax and mutable-style:

immer({a: 1}, state => {
   state.a++
}); 

immer(3, v => v + 10); // 13

@mweststrate
Copy link
Collaborator Author

@AriaFallah i'm currently optimizing the proxy implementation, and it looks very promising :)

#27 (comment)

Which seems to be roughly a 2 fold drop over a hand written reducer (with auto freezing disabled, which a normal reducer also doesn't do ;-)).

@kelvcutler
Copy link

I am doing a project in react native (android first). It’ll be a decently small app, so I decided on doing raw react — no redux or mobx. But when I saw this library, I saw its potential for making things that much cleaner in my code — made me happy!

I swapped in the produce function in place of some manual cloning and merging and it did clean things up :D. However, I went to run it and doing “import produce from ‘immer/es5’” doesn’t work — can’t resolve that library. Sure enough, in node_modules/immer, there doesn’t seem to be any es5 file or folder. I see it in your github src code. I’m not sure why it wouldn’t be there.

In the readme, it reads as though the library should automatically fallback to the es5 version, but that isn’t the case. I removed the “/es5” from the import, running it then gives me a different error, complaining about an "unknown variable: Symbol". I'd love to start using this, but I'm not sure how to best to proceed. Any help would be much appreciated! Thanks!

@mweststrate
Copy link
Collaborator Author

mweststrate commented Jan 17, 2018 via email

@13806
Copy link

13806 commented Feb 17, 2018

Hello! Thanks for immer, it has been a time- and headache-saver so far.

Ive been using produce for deleting stuff, mostly. I'd like to learn on how to properly use it for adding/subtracting stuff too, however. What I'm struggling with is checking whether the key exists without piling colossal amounts of code that I'm used to with normal object spreads.
For example, if I have...

ids: {
    id1: {
            size1: { 
                    color1: { quantity: 1 }
                    color2: { }
                }
            }
            size2: { ... }        
    }
    id2: { ... }
}

...and I want to increment quantity by 1, I'd usually write the reducer like

const add = (state, action) => ({
    ...state,
    ids: {
         ...state.ids,
        [action.id]: {
            ...state.ids[action.id],
            [action.size]: {
                ...state.ids[action.id][action.size],
                [action.color]: {
                    ...state.ids[action.id][action.size][action.color]
                    quantity: (state.ids[action.id][action.size][action.color].quantity || 0) + 1
            }
        }
    }
)}

I'm sure I missed a bracket or a ternary check along the way. Point is, I was hoping for something like

remove.Color = (state, action) =>
	produce(state, draft => {
		draft[action.id][action.size][action.color].quantity ++
	})

However, this needs to check if id exists, if not, then push it, if size exists, if not, then push it, if color exists (else it throws a TypeError)... this ends up with just as much code as the usual object spread version.
I am sure I'm missing something, is there a quick and dirty way of incrementing quantity when the id itself may not pre-exist (in which case, add it and the layers that come packed with the action object and make the quantity default to 1)?

@jarvisluong
Copy link

Your library is really a headache-solver for immutability!!!
Thanks alot!!!

@mweststrate
Copy link
Collaborator Author

@plkuzmich I see your problem, but it is not one that this package tries to solve. There are packages that solve the auto-object creation (can't recall a name though), or you could even write it yourself with proxies, for example: http://slides.com/elektronik/metaprogramming-via-es2015-proxies#/15. However, the problem is not in scope of this package. (And also unsolvable without proxies)

@athomann
Copy link

@13806 I was looking for something similar and ended up using lodash set function.

@celebro
Copy link

celebro commented Apr 5, 2018

I found the use of Object.assign (https://github.com/mweststrate/immer/blob/v1.2.0/src/common.js#L60) a bit problematic. ES5 implementation is partly for IE, but Object.assign doesn't exist in IE either. It's an annoyance when used on a project with babel runtime instead of polyfill.

@mweststrate
Copy link
Collaborator Author

@celebro good catch. Could you open a separate PR or issue for that?

@celebro
Copy link

celebro commented Apr 6, 2018

I'll see if I find the time for PR this weekend.

@wonderwhy-er
Copy link

Pretty cool project, I am pondering using immutable structures in one of my hobby React/Data analysis project but never liked somewhat alien feel of ImmutableJS.
Immer looks way better.

Only missing thing so far for me to move is that it does not support custom classes with getters and functions.

Here is a quick test I run to see how it handles them https://stackblitz.com/edit/react-ts-9u23rs?file=index.tsx

And it just does not work correctly.
I did not had time yet to clone repository and check myself where it does wrong. Whether it is proxy or Object.assign problem. But I do plan to play with code a bit and think of way to add support of classes.

It seems to me it can work in pretty cool way where you have nested models hierarchy with complex nested update logic that all happen inside produce as a large transaction and it is transparent for nested logic.

@wonderwhy-er
Copy link

It seems I hit a roadblock with converting mutable state container to immer and am pondering what to do or how to solve this.

Imagine we have a list of items.
Then we have filtered list of items which is a subset of list above.

In case of state being not normalised filtered list has references to items in whole list.
Then if we run produce changing item in one list it won't change item in other list due to Immer treating structure as a tree where each item only has one parent.

Now solution could be to normalise, add unique id's to items and store only id's in second list.
Sadly this breaks things as well. Now changing items does not mark filtered list as changed.
So if we have a pure function with memoize that only receives filtered list it will not notice a change.
We need to pass in unfiltered list as well now and changes in items not in filtered list will break memoization which is sub optimal.

I am pondering if it is possible to make something similar to immer but with support for multiple parents for items. So far looks to me that it is hard to achieve in optimal way. Anyone else stumbled on problems with this?

@mweststrate
Copy link
Collaborator Author

Nice article: https://medium.com/workday-engineering/workday-prism-analytics-the-search-for-a-strongly-typed-immutable-state-a09f6768b2b5

Closing for now as the usefulness of the package has become pretty clear :)

@mweststrate
Copy link
Collaborator Author

@wonderwhy-er if this is still an open question, please open a separate issue instead.

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

10 participants