ngrx / platform

Reactive State for Angular
https://ngrx.io
Other
8.01k stars 1.97k forks source link

RFC: Allow more than 10 signal store features #4314

Closed kobi2294 closed 2 months ago

kobi2294 commented 5 months ago

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 store with 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

gabrielguerrero commented 5 months ago

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

SonyStone commented 5 months ago

I needed this feature too, so I created a PR

rainerhahnekamp commented 5 months ago

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();

signalStoreBuilder calls internally signalStore. There is already a proof of concept in https://github.com/rainerhahnekamp/ngrx/blob/feat/signals/builder/modules/signals/spec/signal-store-builder.spec.ts

Since the usage of withState/Computed/Methods is so common, there are also utility functions that makes it even easier:

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 add* can be called.

gabrielguerrero commented 5 months ago

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). I'm not sure about the utility functions, I prefer to have one way of doing things if possible via the add( withFeature ) , plus it makes the builder dependent on withState, withMethods, withComputed which I don't like much either

SonyStone commented 5 months ago

Can we add those types first? https://github.com/ngrx/platform/pull/4315 And then do any breaking changes?

rainerhahnekamp commented 5 months ago

The implementations of the Builder are, of course, up for discussion. It is not a breaking change. SincesignalStore is tree-shakable and the builder isn't (thanks to @mikezks), I'd say the signalStore should stay as the primary function to generate a signal store.

At the same time that doesn't mean that we shouldn't extend signalStore and signalStoreFeature.

gabrielguerrero commented 5 months ago

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.

markostanimirovic commented 5 months ago

Thanks everyone for sharing your thoughts! This issue will be considered.

https://github.com/ngrx/platform/pull/4315#issuecomment-2099422708

SonyStone commented 5 months ago

Can we make the parameters with features for signalStore just an array?

signalStore(conf, [
  withState(),
  withMethods(),
  withMethods(),
  [
    withMethods(),
    withMethods(),
  ]
])

And inside signalStore just do:

function signalStore(config: SignalStoreConfig, args: SignalStoreFeature[]) {
  const features = args.flat();
  ...
}

Then it will be possible to group fixtures into arrays and pass them to signalStore

markostanimirovic commented 5 months ago

Can we make the parameters with features for signalStore just an array?

signalStore(conf, [
  withState(),
  withMethods(),
  withMethods(),
  [
    withMethods(),
    withMethods(),
  ]
])

And inside signalStore just do:

function signalStore(config: SignalStoreConfig, args: SignalStoreFeature[]) {
  const features = args.flat();
  ...
}

Then it will be possible to group fixtures into arrays and pass them to signalStore

With existing TypeScript features, it's not possible to provide strong typing for this.

rainerhahnekamp commented 5 months ago

@SonyStone the closest thing to your array proposal is the builder pattern.

SonyStone commented 5 months ago

@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 Observable.pipe? signalStore already have signalStoreFeature, вut it does not have an type input through which it knows information about previous features. Is it passable to add it?

signalStore(
  withState()
  withComputed()
  signalStoreFeature(
    withMethods((store) => ...) // ⇽ cannot get value from `withComputed`
  )
)
rainerhahnekamp commented 5 months ago

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 add and build methods (as @gabrielguerrero suggested), there is nothing left to shake. You need both methods.

gabrielguerrero commented 5 months ago

@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 Observable.pipe? signalStore already have signalStoreFeature, вut it does not have an type input through which it knows information about previous features. Is it passable to add it?

signalStore(
  withState()
  withComputed()
  signalStoreFeature(
    withMethods((store) => ...) // ⇽ cannot get value from `withComputed`
  )
)

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

gabrielguerrero commented 5 months ago

@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). Example of use:

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 https://github.com/gabrielguerrero/platform/blob/signal-store-feature-receive-previous-input/modules/signals/src/signal-store-feature.ts

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.

gabrielguerrero commented 3 months ago

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. Here is an example, with ngrx traits where you can see how easy we reach the 6 limit, and Im not showing the full list of ngrx-traits withEntities related store features

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 You can see an example with two collections here, you can see it's bordering the limits https://github.com/gabrielguerrero/ngrx-traits/blob/main/apps/example-app/src/app/examples/signals/product-shop-page/products-shop.store.ts

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