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

fix(signals): create deep signals for custom class instances #4614

Merged

Conversation

markostanimirovic
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

  • Deep signals are generated by mistake for built-in types: Error, Date, and RegExp.
  • Deep signals are not generated for custom class instances even though they were typed as deep signals.

Closes #4604

What is the new behavior?

  • Deep signals are not generated for built-in types: Error, Date, and RegExp.
  • Deep signals are generated for custom class instances:
class User {
  constructor(readonly name: string) {}
}

const state = signalState({ user: new User('John') });
console.log(state.user()); // { name: 'John' }
console.log(state.user.name()); // 'John'

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Copy link

netlify bot commented Dec 2, 2024

Deploy Preview for ngrx-io canceled.

Name Link
🔨 Latest commit 5d66296
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/675760ca2af6270007a5c3b2

@rainerhahnekamp
Copy link
Contributor

Hi @markostanimirovic, why do we want to support deep signals for class instances? I’m not sure we should “split it up” as we would with a normal object literal.

To me, a class instance is an atomic unit—it has its own state, encapsulated properties, methods that define its logic, and potentially inheritance. Given these characteristics, I don’t see it as a good fit for a deep signal.

image

@markostanimirovic
Copy link
Member Author

Hi @markostanimirovic, why do we want to support deep signals for class instances? I’m not sure we should “split it up” as we would with a normal object literal.

To me, a class instance is an atomic unit—it has its own state, encapsulated properties, methods that define its logic, and potentially inheritance. Given these characteristics, I don’t see it as a good fit for a deep signal.

I fully agree with you! Because of that, this won't be shown in official docs or be recommended anywhere. There are two main reasons for this change:

  • DeepSignal types are already created for custom class instances, but deep signals are not generated. This leads to confusion and bad DX - errors occur in runtime. Since I couldn't find a way to restrict the usage of custom class instances with signalStore/signalState at the type level, it's better to make it work than have runtime errors in my opinion.
  • Compromise - there are still many apps that use classes for models. This would facilitate their migration to @ngrx/signals.

If you remember in the beginning, SignalStore didn't support interfaces, and the state type was restricted to Record<string, unknown>. To support interfaces, we made the state type less strict which has this issue as a consequence.

@markostanimirovic markostanimirovic force-pushed the fix/signals/create-deep-signals-for-custom-class-instances branch from 7588948 to 5d66296 Compare December 9, 2024 21:27
Copy link
Member

@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.

@rainerhahnekamp is this also OK for you? (because you added a comment)

@rainerhahnekamp
Copy link
Contributor

@timdeschryver

@rainerhahnekamp is this also OK for you? (because you added a comment)

All good 👍

@timdeschryver timdeschryver merged commit 4d34dc4 into main Dec 10, 2024
11 checks passed
@timdeschryver timdeschryver deleted the fix/signals/create-deep-signals-for-custom-class-instances branch December 10, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

signalState is not creating DeepSignal property signals when the value object is an instance of a class
4 participants