Skip to content
This repository has been archived by the owner on Nov 21, 2023. It is now read-only.

freeze state in signalState #2

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rainerhahnekamp
Copy link
Contributor

It is currently possible to run mutable changes via the $update function. Those mutable changes do not throw any error, and effects tracking that part of the state are also not executed. The tests are located in the "immutability" tests suite of signal-state.spec.ts.

This change applies Object.freeze to the state, causing an error on mutable changes.

The implementation is an almost identical copy of freeze in NgRx.

Other than that,

  • move the tests to the test directory, following the existing conventions.
  • add withEffect as a test utility method to simplify the tests of effects.

@rainerhahnekamp rainerhahnekamp marked this pull request as ready for review August 8, 2023 09:18
… a function, i.e. state is exposed.

@markostanimirovic: I am not happy with the `@ts-ignore` in `deepFreeze`. Maybe you can find a better way.
@@ -16,6 +17,7 @@ export type SignalStateUpdate<State extends Record<string, unknown>> = {
export function signalState<State extends Record<string, unknown>>(
initialState: State
): SignalState<State> {
deepFreeze(initialState);
Copy link
Owner

Choose a reason for hiding this comment

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

we should perform deepFreeze only in dev mode for better performance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still working on that

src/lib/utils.ts Outdated Show resolved Hide resolved
src/lib/utils.ts Outdated Show resolved Hide resolved
@@ -1,5 +1,4 @@
import {
selectSignal,
Copy link
Owner

Choose a reason for hiding this comment

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

test1.ts and test2.ts files contain "type tests 😅", to make sure that typing works fine after refactoring. When we move this lib to the NgRx repo, they will be converted into real type tests. For now, they contain a few commented lines that use selectSignal so feel free to revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. I've left testEffects there as it is more a testing file and not a test

src/tests/with-effect.ts Outdated Show resolved Hide resolved
- rename withEffect to testEffects
- type assertion in freezing function
- move signal state spec back to original location
…ovides state as readonly

- refactored tests
- added further tests for mutable and immutable changes
@rainerhahnekamp
Copy link
Contributor Author

Some comments from my latest push:

  • $update has DeepReadonly as input which also comes up with a ReadableArray. I think that this improves the DX when immutability is a requirement.
  • I was not aware that Object.freeze is never applied during prod mode. For application types, where performance is not a big topic, that would be an attractive option. Maybe we can create a GitHub issue once SignalStore is in beta. Same is true for ngrx/store.
  • The unit tests should that I can update the mutate the state as well and the effects are still firing correctly. Is there actually any difference between an immutable and mutable change? Looks like the equal checker works for both.

@markostanimirovic
Copy link
Owner

Have you tried to run the playground app? It seems that the signalStore typing is broken. 👀

If you agree, we can keep only the deepFreeze related changes in this PR and consider introducing DeepReadonly and other typing changes separately.

Regarding the { immutabilityCheck: boolean } option in the signalState function, I don't see the use-case when it should be set to false because signalState (and signalStore) support only immutable updates. Therefore, I suggest keeping the same signature as before (without options argument) and calling deepFreeze in dev mode:

if (isDevMode()) {
  deepFreeze(state);
}

Btw, @ngrx/store provides the ability to turn off the state immutability check, but the main reason for that was to ensure backward compatibility when runtime checks were introduced.

@rainerhahnekamp
Copy link
Contributor Author

Yeah, you're right. Let's get that freeze feature in and discuss the rest in a separate PR.

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

Successfully merging this pull request may close these issues.

2 participants