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

feat: add immerPatchState #24

Merged

Conversation

rainerhahnekamp
Copy link
Contributor

Looking forward to your review.

As discussed earlier:

  • Jest is necessary to run TestBed.flushEffects()
  • Minimal Dependency to @ngrx/signals 17.1.1 because of SignalState

`immerPatchState` adds mutable changes
to the Signal Store.

It requires at least version 17.1.1 since
it needs to have access to `SignalState`.
Copy link
Owner

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rainerhahnekamp !

Before approving this, I want to make sure that the build etc is correctly set up. I'll verify this later today/tomorrow.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/signals/index.ts Outdated Show resolved Hide resolved
rainerhahnekamp and others added 4 commits March 12, 2024 17:32
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
}

export function immerPatchState<State extends object>(state: StateSignal<State>, ...updaters: Array<Partial<State & {}> | PartialStateUpdater<State & {}> | ImmerStateUpdater<State & {}>>) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can update the generics to be able to do the following:

  • the second patch updates the state and knows there a request state
  • the first patch does the same but the state gets narrowed down because of the use of setAllEntities

image

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like the following seems to work:

export declare function immerPatchState<State extends object, T = State>(state: StateSignal<T>, ...updaters: Array<Partial<T & {}> | PartialStateUpdater<T & {}> | ImmerStateUpdater<T & {}>>): void;

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Tim, I can't currently work on it, but please update my branch as you wish. I'll continue tomorrow.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can ignore this.
In WebStorm this was working, after setting the TS version of Visual Studio Code to the installed version it does work.
Sorry for the confusion.
Let's ship this! 🚀

@timdeschryver timdeschryver merged commit c3a8833 into timdeschryver:main Mar 13, 2024
4 checks passed
@timdeschryver
Copy link
Owner

Thanks for the implementation @rainerhahnekamp !
And thanks @markostanimirovic for the review!

Copy link

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants