8000 feat(signals): define SignalStore members as readonly by Chiorufarewerin · Pull Request #4713 · ngrx/platform · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(signals): define SignalStore members as readonly #4713

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.

Sign up for GitHub

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? 8000 Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(signals): define SignalStore members as readonly #4713

wants to merge 1 commit into from

Conversation

Chiorufarewerin
Copy link
@Chiorufarewerin Chiorufarewerin commented Mar 1, 2025 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

#4712

What is the new behavior?

After these changes, the store and entities will become type-safe:

this.userStore.entities = signal([{ id: 1000, name: 'Vova' }]); // error
this.userStore.entityMap()[0] = { id: 2000, name: 'Dima' }; // error
this.userStore.entities()[0] = { id: 2000, name: 'Dima' }; // error
this.userStore.entities = signal([]); // error
this.userStore.loadAll = () => undefined; // error

Does this PR introduce a breaking change?

[x] Yes
[ ] No

If someone modifies the store or directly edits entities, they will need to fix these errors (or add just as any). However, this will help prevent future errors by enforcing type safety.

Also, I want to mention that it doesn't introduces "breaking changes" in logic, like it was with Object.freeze that was reverted #4683.

Other information

In my application, almost every type includes readonly to ensure type safety. I always know when this rule is broken—only when using the as keyword. It would be great to have similar safety in @ngrx/signals.

This PR provides the minimum type safety I want. Additionally, I suggest changing every instance like this:

SignalStoreFeature<Input, { state: {}; props: {}; methods: Methods }>

To:

{
    readonly state: {};
    readonly props: {};
    readonly methods: Methods;
  }

I can also implement this change if the idea is accepted.

@netlify Netlify
Copy link
netlify bot commented Mar 1, 2025 •
edited
Loading

✅ Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit fb54f15
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/67c4b05c2d5b8e000837f732

@Chiorufarewerin Chiorufarewerin changed the title feature(signals): Enhance type safety feature(signals): enhance type safety Mar 2, 2025
@Chiorufarewerin Chiorufarewerin force-pushed the feature/signals/enhence-type-safety branch from c0dac48 to fb54f15 Compare March 2, 2025 19:24
rainerhahnekamp
rainerhahnekamp approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor
@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

I don't know if there are any side-effects or edge cases where this might become problematic. I don't see any and I do agree this would be a helpful addition.

The PR looks solid. So green light from my side.

@markostanimirovic
Copy link
Member

This is a breaking change and cannot be considered before v20. I'll rename the PR with a precise title and it will be revisited before v20.

Regarding non-readonly SignalStore members, they could be useful in tests, if someone wants to replace a specific prop with a mocked one:

const myStore = TestBed.inject(MyStore);
myStore.mySignal = signal(10);
// ...

I don't expect anyone to do this in production code, so let's see if introducing this breaking change will be worth it.

@markostanimirovic markostanimirovic changed the title feature(signals): enhance type safety feat(signals): define SignalStore members as readonly Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rainerhahnekamp rainerhahnekamp rainerhahnekamp approved these changes

@markostanimirovic markostanimirovic Awaiting requested review from markostanimirovic

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Breaking Change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Chiorufarewerin @markostanimirovic @rainerhahnekamp
0