ngrx / platform

Reactive State for Angular
https://ngrx.io
Other
8.04k stars 1.98k forks source link

bake in patchState immutability for simplicity #4245

Closed ducin closed 8 months ago

ducin commented 8 months ago

Which @ngrx/* package(s) are relevant/related to the feature request?

signals

Information

The proposal is to use immer or mutative in order to simplify immutable operations maintenance for developers.

Motivation

Signals require immutable updates. For nested structures, writing immutable code by hand is going to be cumbersome for devs. This proposal aims to remove this incovenience.

Example

the "hidden" immutability would be baked in patchState.

BEFORE:

withMethods((store) => ({
  updateQuery(query: string): void {
    patchState(store, (state) => ({ filter: { ...state.filter, query } }));
  },
...

AFTER:

withMethods((store) => ({
  updateQuery(query: string): void {
    patchState(store, (draft) => { draft.query = query; });
  },
...

and this is how mutative/immer normally work:

import { create } from 'mutative';

const baseState = {
  foo: 'bar',
  list: [{ text: 'coding' }],
};

const state = create(baseState, (draft) => {
  draft.list.push({ text: 'learning' });
});

Context

In react ecosystem people have to apply immutable data updates. That was one of the many painpoints with original redux, which introduced the need for quite a lot of boilerplate. The need for immutability in reducers was one of the factors that made redux quite unpopular within react community in last years. Later, @markerikson did a great job by introducing redux-toolkit (opinionated, 50%+ less code to do typical redux in apps), which has reduced the boilerplate significantly. One of the great improvements was introducing immer as a hard dependency. All redux-toolkit users I know are super happy with how immer removes the need for spreading the objects over and over again.

Drawbacks

The only downside I'm seeing is potentially the bundle size which currently is super small.

image

Describe any alternatives/workarounds you're currently using

Let people write spread objects as they are doing now.

I would be willing to submit a PR to fix this issue

OF COURSE (:

rainerhahnekamp commented 8 months ago

There is already a similar issue, which is more about a compromise: https://github.com/ngrx/platform/issues/4030

We would have immutability on a type level and don't require immer. I created a PR for the prototype of the NgRx Signal Store. If there is interest, I could bring it up again.

brandonroberts commented 8 months ago

Left some context in that issue also, but we've discussed this issue over time https://github.com/ngrx/platform/issues?q=is%3Aissue+immer+is%3Aclosed+

markostanimirovic commented 8 months ago

The immerPatchState function will be added to the ngrx-immer package.

immerPatchState(store, (state) => {
  state.todos.push('foo');
});

Closing this issue in favor of https://github.com/timdeschryver/ngrx-immer/issues/21

timdeschryver commented 8 months ago

I'm using this issue to promote https://github.com/timdeschryver/ngrx-immer/issues/21 If you want to contribute feel free to drop your name there, and it will get assigned to you

rainerhahnekamp commented 8 months ago

@timdeschryver, I would take that one. So if there's a waiting list out there, please add me.

ducin commented 8 months ago

@rainerhahnekamp @markostanimirovic @timdeschryver FWIW I have just implemented (or rather, bridged) it here: https://github.com/ducin/sygnalyze. sygnalyze is meant to be a multi-purpose angular signals toolkit, also (but not only) with ngrx signal store extensions (in a separate entry). But having nothing to do with rxjs-based ngrx.

Although it very slightly overlaps with ngrx-immer, in the future maybe(?) also with angular architects ngrx toolkit (though not gonna copy anything from anywhere) let's treat it as a healthy competition and inspire each other 🍻 🤝

rainerhahnekamp commented 8 months ago

@ducin, good, so I understand that I can still extend ngrx-immer and we might have multiple community solutions.

ducin commented 8 months ago

yeah, I think so