-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(signals): Protection for Overriding Properties #4199
base: main
Are you sure you want to change the base?
feat(signals): Protection for Overriding Properties #4199
Conversation
This Proof of Concept intrdocues a feature which prevents that feature override properties (`state`, `signals`, `methods`). It does that by adding a conditional type `NoOverride` which as a constraint to the parameters of `signalState`. Example: signalState( withState({ id: 1, name: 'hallo', prettyName: 'hi' }), withComputed((store) => { return { prettyName: computed(() => store.name()), }; }) ) This would fail to compile because `prettyName` is overriden. The overriding protection is enabled by default. It is very likely that overriding doesn't happen on purpose. An opt-out is also possible via `signalState({allowOverrides: true})`.
✅ Deploy Preview for ngrx-io ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@markostanimirovic, since I don't know which process you prefer, I only created a PR. If you prefer to open an issue first and do all the discussions there, please let me know. |
* The overriding protection is enabled by default. It is very likely | ||
* that overriding doesn't happen on purpose. | ||
* | ||
* An opt-out is also possible via `signalState({allowOverrides: true})`. |
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.
First of all thanks for all these PRs and feedback @rainerhahnekamp.
If we go through with this change, what would be the benefit of having this configurable?
Is there a use case where this would be desired to turn this off, and override properties?
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.
Hi Tim, thanks a lot as well.
To me, the use case for disabling the override protection would be in enterprise-like environments, where teams share huge extensions and I - as a consumer - want to customize or change parts of them. So a little bit similar to what we can do with class inheritance.
For example, I'd like to override undo
of a withUndoRedo
extension:
signalStore(
// ...
withUndoRedo(),
withMethods(store => {
return {
undo: (times = 1) => {
for(const i = 0; i < times; i++) {
store.undo();
}
}
}
})
);
Maybe it doesn't have to be a configuration value but it could be a function we can add selectively:
signalStore(
// ...
withUndoRedo(),
withOverride(
withMethods(store => {
return {
undo: (times = 1) => {
for(const i = 0; i < times; i++) {
store.undo();
}
}
}
})
)
);
If we go through with this change...
Before we add new features to https://github.com/angular-architects/ngrx-toolkit, I'd like to post them here first.
If you see it as something which should be part of the core, we can move forward here. If you see, it is a better fit for a community contribution, we would integrate it into the toolkit. Could also serve as an incubator.
In the same manner, I also plan to push the branch for an encapsulation feature.
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.
Thanks for the elaboration @rainerhahnekamp
This Proof of Concept intrdocues a feature which prevents that feature override properties (
state
,signals
,methods
).It does that by adding a conditional type
NoOverride
which as a constraint to the parameters ofsignalState
.Example:
This would fail to compile because
prettyName
is overriden.The overriding protection is enabled by default. It is very likely that overriding doesn't happen on purpose.
An opt-out is also possible via the configuration:
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Features can override properties of other features.
Closes #
What is the new behavior?
By default we get a compilation error. Allowing overrides is possible via the configuration.
Does this PR introduce a breaking change?
Honestly, I think that developers would welcome that breaking change. Overriding properties can cause bugs which might be hard to detect.
Other information
This is just a POC. If accepted, it has to be integrated into the original
withState
and probablysignalStoreFeature
.