-
Notifications
You must be signed in to change notification settings - Fork 26.7k
feat(core): make SimpleChanges generic #64535
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
Conversation
Moves the `SimpleChange` and lifecycle hook interfaces so that they can import other symbols from `core`.
Currently it's easy to make a mistake when accessing properties on `SimpleChanges`, because the keys aren't typed. These changes add an optional generic to the interface so that users can get a compilation error if they make a typo. A few things to note: 1. The generic argument is optional and we revert to the old behavior if one isn't passed for backwards compatibility. 2. All of the keys are optional, because they aren't guaranteed to be present for any `ngOnChanges` invocation. 3. We unwrap the values of input signals to match the behavior at runtime. Fixes angular#17560.
c691b52 to
ae7eda8
Compare
| export type SimpleChanges<T = unknown> = T extends object | ||
| ? { | ||
| [Key in keyof T]?: SimpleChange< | ||
| T[Key] extends {[ɵINPUT_SIGNAL_BRAND_READ_TYPE]: infer V} ? V : T[Key] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know we have a helper for this already? (e.g. for type checking?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK we don't, I only found the function that asserts if a type is a writable signal. Initially I wanted to use the public InputSignal type, but it would've gotten complicated since we have some variations like InputSignalWithTransform.
|
Passing TGP after some client app cleanups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed-for: public-api
| expect(ngModel.control.disabled).toEqual(false); | ||
|
|
||
| ngModel.ngOnChanges({isDisabled: new SimpleChange('', false, false)}); | ||
| ngModel.ngOnChanges({isDisabled: new SimpleChange('' as any, false, false)}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to cast those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because the types for the previous and current value don't match. I think it's testing something that isn't officially supported through types.
This comment was marked as outdated.
This comment was marked as outdated.
Currently it's easy to make a mistake when accessing properties on `SimpleChanges`, because the keys aren't typed. These changes add an optional generic to the interface so that users can get a compilation error if they make a typo. A few things to note: 1. The generic argument is optional and we revert to the old behavior if one isn't passed for backwards compatibility. 2. All of the keys are optional, because they aren't guaranteed to be present for any `ngOnChanges` invocation. 3. We unwrap the values of input signals to match the behavior at runtime. Fixes #17560. PR Close #64535
|
This PR was merged into the repository. The changes were merged into the following branches:
|
Currently it's easy to make a mistake when accessing properties on
SimpleChanges, because the keys aren't typed. These changes add an optional generic to the interface so that users can get a compilation error if they make a typo.A few things to note:
ngOnChangesinvocation.Fixes #17560.