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.
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.
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.
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
NoOverridewhich as a constraint to the parameters ofsignalState.Example:
This would fail to compile because
prettyNameis 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
withStateand probablysignalStoreFeature.