-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
RFC: Allow more than 10 signal store features #4314
Comments
@markostanimirovic , I think this is a good idea. I faced this and know people who had this problem too, it is easy to reach the max 9 params, and in signalStoreFeature is easy to reach the max 6 it has. As mentioned, it has a workaround of using signalStoreFeature to group it, but it would be nicer if it had more params; if you guys agree this should be implemented, I am happy to help create a PR. |
I needed this feature too, so I created a PR |
I would like to bring in an alternative. We could apply a builder pattern. Its implementation would allow us to have as many features as we want. It would look like this const Store = signalStoreBuilder()
.add(withState({ counter: 1 }))
.add(
withMethods((store) => {
return {
increment() {
patchState(store, ({ counter }) => ({ counter: counter + 1 }));
},
};
})
)
.add(
withComputed((state) => {
return {
double: computed(() => state.counter() * 2),
};
})
)
.build();
Since the usage of const Store = signalStoreBuilder()
.addState({ counter: 1 })
.addMethods((store) => {
return {
increment() {
patchState(store, (value) => ({ counter: value.counter + 1 }));
},
};
})
.addComputed((state) => ({
double: computed(() => state.counter() * 2),
}))
.build(); Again, there is no limitation on how often |
I liked @rainerhahnekamp idea, it handles infinite composition, plus simplifies the types. I can not see any cons of this approach, besides it being a breaking change if it is decided the old way will be deprecated (both could be supported, but I think is better to stick with one). |
Can we add those types first? |
The implementations of the Builder are, of course, up for discussion. It is not a breaking change. Since At the same time that doesn't mean that we shouldn't extend |
Yeah I can see the builder as an extra way of doing things for more complex scenarios. I am just not sure if is worth adding an extra api instead of just adding more params to the current way, especially because I think TS will eventually have a nice way of implementing the infinite params, I tried to implement it but there are some limitations currently with using infer with types that have generics that makes it lose types. |
Thanks everyone for sharing your thoughts! This issue will be considered. |
Can we make the parameters with features for signalStore(conf, [
withState(),
withMethods(),
withMethods(),
[
withMethods(),
withMethods(),
]
]) And inside function signalStore(config: SignalStoreConfig, args: SignalStoreFeature[]) {
const features = args.flat();
...
} Then it will be possible to group fixtures into arrays and pass them to |
With existing TypeScript features, it's not possible to provide strong typing for this. |
@SonyStone the closest thing to your array proposal is the builder pattern. |
@rainerhahnekamp With builder pattern I don't know how to group features together and the builder pattern isn't tree-shakable. Maybe some kind of signalStore(
withState()
withComputed()
signalStoreFeature(
withMethods((store) => ...) // ⇽ cannot get value from `withComputed`
)
) |
I don't see the need for adding a grouping feature, but if you find it necessary, we could add it to the builder. In terms of tree-shaking: If it is only about the |
signalStoreFeature has a first param where you can define the previous types it depends on signalStoreFeature(type<{
state: EntityState<Entity>;
signals: EntitySignals<Entity>;
methods: {};
}>(), withMethods,.... but yeah it does not auto-get the previous type of the store in the withMethods use inside, I think I have an idea that could make that work, I will investigate |
@markostanimirovic, I did an investigation into making signalStoreFeature get the type of the store when it is inline, so it works similar to rxjs pipe operator. I managed to make it work in the fork branch at the end of the comment, is backwards compatible, all tests in the signals project pass (great tests by the way it really help me find all corner cases), I also added an extra one to test the inline case (I could add more if you guys want). const Store = signalStore(
withCustomFeature1(),
withComputed(({ bar }) => ({
s: computed(() => bar() + 's'),
})),
signalStoreFeature(
withState({ foo2: 'foo2' }),
withState({ bar2: 'bar2' }),
withComputed(({ bar2 }) => ({
s2: computed(() => bar2() + 's'),
})),
withMethods(({ foo, bar, baz, s, foo2, bar2, s2 }) => ({
all: () => foo() + bar() + baz() + s() + foo2() + bar2() + s2(),
}))
)
); Notice that the last withMethods can access everything defined before it, regardless of whether it is part of the parent store or another previous store feature. The only thing is that it requires Ts 5.4 NoInfer, so we might need to wait until the Angular 18 release if you guys think this should be added. Below is the fork, I did not create a PR because not sure if you guys want this in or maybe want to do your own version; let me know what you think The main changes are in signal-store-feature.ts, and a small change in signal-store-models.ts to MergeTwoFeatureResults, let me know any questions. |
Hey guys, have you given any thoughts on this ticket? It might help to show the problem in ngrx-traits. Users seem to hit these limitations quite often. I think the one that bothers them most is the signalStoreFeature limit to a maximum of 6 params. It should at least be equal to signalStore 10, I believe. const orderEntity = type<ProductOrder>();
const orderItemsCollection = 'orderItems';
const orderItemsStoreFeature = signalStoreFeature(
withEntities({
entity: orderEntity,
collection: orderItemsCollection,
}),
withEntitiesLocalSort({
entity: orderEntity,
collection: orderItemsCollection,
defaultSort: { field: 'name', direction: 'asc' },
}),
withEntitiesMultiSelection({
entity: orderEntity,
collection: orderItemsCollection,
}),
withEntitiesLocalPagination({
pageSize: 10,
entity: orderEntity,
collection: orderItemsCollection,
}),
withStateLogger({
name: 'orderItemsStore',
filterState: ({ orderItemsEntityMap }) => ({ orderItemsEntityMap }),
}),
withSyncToWebStorage({
key: 'orderItems',
type: 'session',
filterState: ({ orderItemsEntityMap, orderItemsIds }) => ({
orderItemsEntityMap,
orderItemsIds,
}),
}),
); And in the store you want to add two collections you can easily reach the 10 , the only option is to split them in signalStoreFeatures, but even those can get to the limit easy as shown before In the case of user not using ngrx-traits , I think they are creating granular businnes operations like withAddUser, withDeleteUser, withProductList and so on , and mixing them they also start facing these problems |
Which @ngrx/* package(s) are relevant/related to the feature request?
signals
Information
The typescript definition of
signalStore
function contains 10 overrides, which allows a maximum of 10 signal storewith
methods to be used. That includes all the custom features and the features they are using. In real life scenario I have about 20 reusable custom features that can be used in my signal stores, but when I combine more than 3-4 of them together, the typescript compiler marks errors becuase there is no overload for a type that merges more than 10 feature results.Describe any alternatives/workarounds you're currently using
I am combining sets of custom features together to reduce the total amount of
with
methods used. So one custom feature actually adds 2 features at the same time, with merged state, merged sets of computed signals, and so on. This of course reduces the modularity of the application - which goes against the whole idea of custom features (which is brilliant, BTW)I think a more realistic limit to the number of merged features results is about 25 :-)
I would be willing to submit a PR to fix this issue
The text was updated successfully, but these errors were encountered: