one-app icon indicating copy to clipboard operation
one-app copied to clipboard

Switch to immer from immutable-js

Open sklawren opened this issue 4 years ago • 12 comments

💡 Feature request

Since we are upgrading to react-redux 7.x in the v5 pilot program, it follows that we should also switch to immer from ImmutableJS.

Immer is the immutable data library used by the authors of react-redux in their starter kit, and they strongly recommend in their documentation to use immer with react-redux and to not use ImmutableJS anymore.

https://redux.js.org/style-guide/style-guide#use-plain-javascript-objects-for-state

Prefer using plain JavaScript objects and arrays for your state tree, rather than specialized libraries like Immutable.js. While there are some potential benefits to using Immutable.js, most of the commonly stated goals such as easy reference comparisons are a property of immutable updates in general, and do not require a specific library. This also keeps bundle sizes smaller and reduces complexity from data type conversions.

https://redux.js.org/style-guide/style-guide#use-immer-for-writing-immutable-updates

Writing immutable update logic by hand is frequently difficult and prone to errors. Immer allows you to write simpler immutable updates using "mutative" logic, and even freezes your state in development to catch mutations elsewhere in the app. We recommend using Immer for writing immutable update logic.

Describe the solution you'd like

I believe that immer should be the default in v5, with an option flag to build a module using ImmutableJS, for projects where backwards-compatibility is necessary.

Additional context

I have a lot of experience using immer with react-redux and I'm happy to contribute to the project if it would help.

The react-redux starter kit might be too limiting for us, but making immer work globally for reducers is incredibly easy. This is the entirety of the code that is required to make reducers use immer:

import createNextState from 'immer';

const immerse = reducer => (state, action) =>
    createNextState(state, draft => reducer(draft, action));

const immerseReducers = reducers => Object.keys(reducers).reduce((acc, key) => {
    acc[key] = immerse(reducers[key]);
    return acc;
}, {});

When the module author passes their reducers object, this takes care of making them immer-ready.

sklawren avatar Jan 29 '20 04:01 sklawren

I'm a Redux maintainer, and I strongly endorse this idea :)

(Also, if Redux Toolkit isn't sufficient for your needs, I'd be interested in hearing more about the use cases you're working with.)

markerikson avatar Jan 29 '20 04:01 markerikson

The One App core team is definitely interested in alternatives to ImmutableJS given the current status of that project. We are definitely interested in the Immer library as an alternative. Given our current roadmap of features we wish to deliver in One App v5, we will be unable to tackle a migration to a new immutable data structure library until we start work on our next major version (v6) later this year. We would be interested in having community contribution to make this migration happen across One App’s ecosystem of supporting libraries (e.g Holocron, one-app-cli, one-app-ducks, Iguazu, Vitruvius) as well as One App itself at that time.

mtomcal avatar Jan 29 '20 21:01 mtomcal

This issue is stale because it has been open 30 days with no activity. Remove no-issue-activity label or comment or this will be closed in 5 days.

github-actions[bot] avatar Mar 19 '20 00:03 github-actions[bot]

bump :)

markerikson avatar Mar 19 '20 01:03 markerikson

This issue is stale because it has been open 30 days with no activity.

github-actions[bot] avatar Apr 19 '20 00:04 github-actions[bot]

bumpity

markerikson avatar Apr 19 '20 02:04 markerikson

Note for later, there are some existing codemods to support this https://github.com/quintoandar/farewell-immutablejs

10xLaCroixDrinker avatar Nov 03 '21 15:11 10xLaCroixDrinker

Hi guys, lagger here, just to confirm, if I use the immerser wrapper, then the whole code base should suddenly use Immer, even if I do not use them within each reducer? The immutability is guaranteed by wrapping the whole thing so that "state" is a draft object? This sounds awesome, thanks for sharing this piece of code. Instead of codemods, I am thinking about implementing a "non-immutable", that basically uses Immutable API but rewritten with basic lodash functions, such as set to replace setIn. I think with your wrapper it should work immediately.

Edit I've progressed a bit, here is what I've done in case others need the same migration:

  • using "redux-immer" to combine reducers
  • modifying slightly the combine function to add a "fromJS" call
  • replacing "immutable" by a non-immutable package, that adds relevant methods to objects. This helps temporarily replacing things such as "set", "setIn" etc. However after having spent an afternoon on this, I think it's faster to do search and replace, if your codebase is not a zillion line of immutable calls

I am still curious whether I should make each reducer immutable, or only the top one. The thing is that, during testing, I need reducers to be self contained, so each should call "immerse". But I wonder if it's good in terme of performance, compared to having a top level call to immer instead.

eric-burel avatar Jan 18 '22 10:01 eric-burel

@eric-burel could you share info, if it is possible to have immutablejs and immer at same time? I'm planning a migration but estimated time is around 3-4 weeks - i need some goooood strategy :)

piciuok avatar Feb 06 '22 19:02 piciuok

@piciuok : sure, you just have to handle each chunk of state accordingly. (You can even have Immutable.js values inside Redux Toolkit's Immer-powered createSlice, because that has always let you do your own immutable updates and return the new values.)

In general, I'd suggest wrapping the usages of the Immutable objects in selectors so that the rest of the codebase isn't trying to call myMap.get() directly, etc, then migrating that bit of state from Immutable.js to Immer.

markerikson avatar Feb 06 '22 20:02 markerikson

@piciuok Here is a gist, I've replaced all imports to immutable by this "non-immutable"

  • Link: https://gist.github.com/eric-burel/66c1f531d0dd987a99d6b6ff3bd1a184
  • /!\ It may be buggy! I've noticed a few minor issues with initial state in particular, I used a clone here and there that I didn't need when using Immutable, eg when resetting the project.
  • So migrating out of immutable one function at a time is still the best option
  • Unit tests really really helped. Don't migrate smth that you did not unit test. Since you use Immutable, write helpers in the tests to avoid any actual call to immutable (then you can ditch the test helpers/change them to remove Immutable for good)
  • The reducer are "Immersed" at the top-level. So during unit tests, when you import a single reducer, you need to "Immerse" it as well.

I don't use redux-toolkit (the app is really old :)) so I cannot help with the specific syntax of it though. It took me smth like 2 days of work, so not that bad, and it's worth it.

eric-burel avatar Feb 07 '22 16:02 eric-burel

@eric-burel - oh, thanks for gist code! I will try it in the comming weeks. My project is from late 2018 and we haven't Redux Toolkit too. My main reasons for change that library (Immutable.js) are readbility, types issues and.... Hooks in React.

@markerikson Hm, so root level of Redux store is just plain object with keys paired with something what manage data? I use reselect and use it for accessing store data, so that should be fine if i migrate reducer by reducer in app - it look like time saving solution :D I planned to do it on side branch but if someone develop something new in app reducers it will cause problem when i start merging with main branch. Thanks for tips!

piciuok avatar Feb 07 '22 19:02 piciuok