Skip to content
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: signalStoreFeature and access to Store #4340

Closed
1 of 2 tasks
rainerhahnekamp opened this issue May 14, 2024 · 23 comments
Closed
1 of 2 tasks

RFC: signalStoreFeature and access to Store #4340

rainerhahnekamp opened this issue May 14, 2024 · 23 comments

Comments

@rainerhahnekamp
Copy link
Contributor

rainerhahnekamp commented May 14, 2024

Which @ngrx/* package(s) are relevant/related to the feature request?

signals

Information

This is a proposal for connector function which connects a signalStore to a feature and provides the feature access to store-specific elements.

A StackBlitz example is available here: https://stackblitz.com/edit/ngrx-signal-store-starter-wkuxdh


Sometimes, our Signal Store Features require features from the store they should be plugged in. Since features should be re-usable, direct coupling is impossible.

Following use case. A feature requires access to a loading function which is defined in withMethods of the store.

We have the following feature

function withEntityVersioner<Entity>(loader: () => Observable<Entity[]>) {
  return signalStoreFeature(
    withState({ version: 1, entities: new Array<Entity>() }),
    withMethods((store) => {
      return {
        update: rxMethod<unknown>(
          pipe(
            switchMap(() => loader()),
            filter((entities) => entities !== store.entities()),
            tap((entities) =>
              patchState(store, (value) => ({
                entities,
                version: value.version + 1,
              })),
            ),
          ),
        ),
      };
    }),
    withHooks((store) => ({ onInit: () => store.update(interval(1000)) })),
  );
}

And a store which wants to use it:

interface Person {
  firstname: string;
  lastname: string;
}

signalStore(
  withMethods(() => {
    const httpClient = inject(HttpClient);
    return {
      load() {
        return httpClient.get<Person[]>('someUrl');
      },
    };
  }),
  withEntityVersioner(() => store.load()) // does not compile
);

The proposed feature is about a function withFeatureFactory which connects an autonomous feature to a specific store.

signalStore(
  withMethods(() => {
    const httpClient = inject(HttpClient);
    return {
      load() {
        return httpClient.get<Person[]>('someUrl');
      },
    };
  }),
  withFeatureFactory((store) => withEntityVersioner(() => store.load())),
);

The type of withFeatureFactory would look like this:

export declare function withFeatureFactory<
  Input extends SignalStoreFeatureResult,
  Feature extends SignalStoreFeatureResult,
>(
  featureSupplier: (
    store: Prettify<
      SignalStoreSlices<Input['state']> &
        Input['signals'] &
        Input['methods'] &
        StateSignal<Prettify<Input['state']>>
    >,
  ) => SignalStoreFeature<EmptyFeatureResult, Feature>,
): SignalStoreFeature<Input, EmptyFeatureResult & Feature>;

This feature would be helpful in the discussion #4338.

Although, it is different use case, it is very similar to the suggestion of @gabrielguerrero in #4314 (comment)

Describe any alternatives/workarounds you're currently using

The alternative is that the load function in the example would come from a service which the signalStoreFeature has to inject.

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@gabrielguerrero
Copy link
Contributor

@rainerhahnekamp I had a need for this in an app, and I gave it a go at trying to implement it, I got it working, and then just noticed you had a version in stackblitz, my version has a few improvements like it keeps the wrapped feature input type in case you need it as well

import { SignalStoreFeature } from '@ngrx/signals';
import type {
  SignalStoreFeatureResult,
  SignalStoreSlices,
} from '@ngrx/signals/src/signal-store-models';
import type { StateSignal } from '@ngrx/signals/src/state-signal';
import { Prettify } from '@ngrx/signals/src/ts-helpers';

export function withFeatureFactory<
  Input extends SignalStoreFeatureResult,
  Feature extends SignalStoreFeature<any, any>,
>(
  featureFactory: (
    store: Prettify<
      SignalStoreSlices<Input['state']> &
        Input['signals'] &
        Input['methods'] &
        StateSignal<Prettify<Input['state']>>
    >,
  ) => Feature,
): SignalStoreFeature<
  Input & (Feature extends SignalStoreFeature<infer In, any> ? In : never),
  Feature extends SignalStoreFeature<any, infer Out> ? Out : never
> {
  return ((store: any) => {
    const { slices, methods, signals, hooks, ...rest } = store;
    return featureFactory({
      ...slices,
      ...signals,
      ...methods,
      ...rest,
    } as Prettify<
      SignalStoreSlices<Input['state']> &
        Input['signals'] &
        Input['methods'] &
        StateSignal<Prettify<Input['state']>>
    >)(store);
  }) as Feature;
}

@rainerhahnekamp
Copy link
Contributor Author

Thanks, I never used it honstly. Was just a quick prototype.

In our extensions we still depend on the non-public types. This feature would improve the situation, but only if it lands in the core (also depends on internal types).

@gabrielguerrero
Copy link
Contributor

That's a good point. I also use non-public types in a few places, but this is mainly because I need to allow methods to use things from the store. If this were part of the core, it would reduce the need to expose those non-public types.

@gabrielguerrero
Copy link
Contributor

Having thought on this more, maybe this could be an overload of the signalStoreFeature like

signalStore(
  withMethods(() => {
    const httpClient = inject(HttpClient);
    return {
      load() {
        return httpClient.get<Person[]>('someUrl');
      },
    };
  }),
  signalStoreFeature((store) => withEntityVersioner(() => store.load())),
);

@GuillaumeNury
Copy link

I would love to write:

signalStore(
  withMethods(() => {
    const httpClient = inject(HttpClient);
    return {
      load() {
        return httpClient.get<Person[]>('someUrl');
      },
    };
  }),
  withEntityVersioner((store) => store.load()),
);

But as I asked in #4272, it requires to expose more types to the public API.

@rainerhahnekamp
Copy link
Contributor Author

@gabrielguerrero, I think - and please correct me if I'm wrong - we are talking here about two different features.

  • You need more feature functions and want to fix that by using signalStoreFeature because that's the closest thing at hand. So your usage of signalStoreFeature is bound to a specific signalStore.
flowchart LR
  signalStoreFeature --> Service --> signalStore
Loading
  • I was speaking of providing a genericsignalStoreFeature which needs to get features of an existing signalStore by not depending on it.
flowchart LR
  ServiceImplementation --> signalStore
  withFeatureFactory --> GenericService
  withFeatureFactory --> ServiceImplementation
  signalStoreFeature --> GenericService
Loading

I'd we need both but maybe not under the same name.

@gabrielguerrero
Copy link
Contributor

gabrielguerrero commented Jun 10, 2024

Hey @rainerhahnekamp, I was indeed referring to having the withFeatureFactory be just an overloaded in signalStoreFeature, my reasoning was that the names are similar and I was not sure about the name withFeatureFactory when its main goal is just to give access to the store to another store feature, I don't want people to think they can be use to create store features, that's signalStoreFeature job, which is where I got the idea of maybe making it part signalStoreFeature, an overloaded version that receives factory function with the store as param. I'm just brainstorming, really; I'm not really sure it is a good idea; maybe we just need a different name for withFeatureFactory.

@Donnerstagnacht
Copy link

Donnerstagnacht commented Jun 13, 2024

Is this proposal also intended to enable other signalStoreFeatures to manipulate the state of a store?

Something like:

const counterStore = signalStore(
  { providedIn: 'root' },
  withState(intitialState),
  witSharedState((store) => withIncrement<counter>('items', store))
);
function withIncrement<Entity extends object>(
  key: keyof Entity,
  sharedStoreIn: StateSignal<Entity>
) {
  return signalStoreFeature(
    withMethods(() => {
      const sharedStore = sharedStoreIn as any; //How to type the store?
      return {
        withIncrement() {
          patchState(sharedStore, { [key]: sharedStore[key]() + 1 });
        },
      };
    })
  );
}

Even if it is not intended, could someone help me to type out that use case?
https://stackblitz.com/edit/ngrx-signal-store-starter-23tsna?file=src%2Fmain.ts

Probably, it could also be an idea to let developers choose if they want to share store elements and offering a system like "shareState()", "shareMethods(), "shareStore()".

@rainerhahnekamp
Copy link
Contributor Author

@Donnerstagnacht

you find a working and typed solution at https://stackblitz.com/edit/ngrx-signal-store-starter-z3q5i5?file=src%2Fmain.ts

Your withIncrement will not get the store but an wrapper function which patches the state. By doing that we can decouple withIncrement from the specific store and make it therefore reusable.

@GuillaumeNury
Copy link

@Donnerstagnacht you can also do it with already available feature store Input condition (as documented here)

https://stackblitz.com/edit/ngrx-signal-store-starter-rlktqe?file=src%2Fmain.ts

@GuillaumeNury
Copy link

@rainerhahnekamp in your last example, it seems weird to move the updaterFn out of the custom feature. I like the withFeatureFactory, but I'd rather use it like this:

https://stackblitz.com/edit/ngrx-signal-store-starter-qedtku?file=src%2Fmain.ts

But a type SignalStoreFeatureStore would be needed (I am not sure about the naming of this type 😅)

@rainerhahnekamp
Copy link
Contributor Author

Hi @GuillaumeNury, yeah that's much better.

In the end, it is the old story about getting access to the internal types, right?

@rainerhahnekamp
Copy link
Contributor Author

@GuillaumeNury, @Donnerstagnacht: I've updated my version. Turned out that the only type we really need is SignalState and that one is public.

So we can use the version of @GuillaumeNury and not violate the encapsulation.

@GuillaumeNury
Copy link

@rainerhahnekamp kind of. But with SignalState + 'withFeatureFactory' we can keep internals hidden 🎉

I changed the generic of withIncrement to allow the state to have non-numeric properties : https://stackblitz.com/edit/ngrx-signal-store-starter-pozgde?file=src%2Fmain.ts

The result looks wonderful! I hope to see this in v18.X 😁

@GuillaumeNury
Copy link

GuillaumeNury commented Jun 17, 2024

@rainerhahnekamp I tried to implement again the withTabVisibility feature, using withFeatureFactory and I do not need internal types anymore 😍

https://stackblitz.com/edit/ngrx-signal-store-starter-dmybgr?file=src%2Fmain.ts

The only change is going from:

signalStore(
  withTabVisibility({
    onVisible: (store) => store.loadOpportunity(store.opportunityId()),
  }))

to:

signalStore(
  withFeatureFactory((store) =>
    withTabVisibility({
      onVisible: () => store.loadOpportunity(store.opportunityId()),
    }),
  )
)

@rainerhahnekamp
Copy link
Contributor Author

There is no need for a separate withFeatureFactory.

As described in https://ngrx.io/guide/signals/signal-store/custom-store-features#example-4-defining-computed-props-and-methods-as-input, the shown example can easily be solved with the existing functionalities:

interface Person {
  firstname: string;
  lastname: string;
}

function withEntityVersioner<Entity>() {
  return signalStoreFeature(
    { methods: type<{ loader: () => Observable<Entity[]> }>() }, // <-- that's the key (and typesafe)
    withState({ version: 1, entities: new Array<Entity>() }),
    withMethods((store) => {
      return {
        update: rxMethod<unknown>(
          pipe(
            switchMap(() => store.loader()),
            filter((entities) => entities !== store.entities()),
            tap((entities) =>
              patchState(store, (value) => ({
                entities,
                version: value.version + 1,
              }))
            )
          )
        ),
      };
    }),
    withHooks((store) => ({ onInit: () => store.update(interval(1000)) }))
  );
}

signalStore(
  withMethods(() => {
    const httpClient = inject(HttpClient);
    return {
      loader() {
        return httpClient.get<Person[]>('someUrl');
      },
    };
  }),
  withEntityVersioner() // does not compile
);

@GuillaumeNury
Copy link

@rainerhahnekamp it is indeed possible to use what is currently available, but the DX does not feel good. What if I want multiple withEntityVersioner? How to group code that should be grouped inside the same block?

IMHO, loader method should be "inside" withEntityVersioner, not before:

signalStore(
  withFeatureFactory(
    (store, httpClient = inject(HttpClient)) =>
      withEntityVersioner({
        loader: () => httpClient.get<Person[]>('someUrl')
      })
  )
);

@rainerhahnekamp
Copy link
Contributor Author

@GuillaumeNury, I understand. The question is whether that use case happens so often that it verifies a new feature.

I could update my example and re-open this issue. What's your opinion on that?

@GuillaumeNury
Copy link

For more advanced features, I find the state/method requirements not very ergonomic. I think that exposing a withFeatureFactory function allows to keep all internals hidden: cleaner and smaller public API!

@markostanimirovic
Copy link
Member

The withFeatureFactory helper can be useful for advanced features that need dynamic access to the store. However, for better DX and full flexibility, dynamic access should be handled on the custom feature side.

Take a look at the implementation of withTabVisibility feature: #4272 (comment)

@gabrielguerrero
Copy link
Contributor

gabrielguerrero commented Jul 16, 2024

Hey @markostanimirovic there is a use case that I struggle a bit to find a nice solution that withFeatureFactory would help, I been asked by users of ngrx-traits
that it would be nice if they could set some of the config props of for example withEntitiesLocalSort with previous values for example set the default sort value from a cookie or service, to do that
I been investigating receiving a factory function that receives the store and returns the config like below, similar to what you do in withMethods or I do with withCalls

export function withEntitiesLocalSort<
  Entity,
  Collection extends string,
>(configFactory: (store: SignalStore<Input) => {
  defaultSort: Sort<Entity>;
  entity: Entity;
  collection: Collection;
  idKey?: EntityIdKey<Entity>;
}): SignalStoreFeature<Input, any>{


const config = configFactory(store) // need the store 
}

I keep finding the best way is like you do in withMethods which is pretty similar to what withFeatureFactory does (maybe a better name for it could be withStore ) but it uses internal stuff. The main reason the solution you did in the previous comment withTabVisibility would not work in this case is that I will need to execute the configFactory inside each withState, withMethods or withHooks that is inside the feature which is not ideal, or create a getConfig withMethod that evaluates the configFactory and exposes a new method. Another problem is that withState has a factory version but doesn't receive the store (maybe that could be a feature request on its own). I tried a few other ways where I tried to avoid using internals, but all ended up more complicated than what withMethods and withFeatureFactory do to transform the InternalStore to SignaSource.

I think withFeatureFactory(or withStore :) ) or something similar will be a nice way to hide the internals of transforming the inner store to signalsource.
Other things that could help if maybe we had a way to have internal only values that can be read inside the custom feature, like that idea of using _ to hide props and methods, or maybe allowing to have symbol as keys for methods or props or some sort some like withContext(store => any).

@markostanimirovic
Copy link
Member

Thanks for the info, Gabriel!

We're considering introducing another base feature - withProps that would allow adding properties to the SignalStore without type restrictions. Although this feature would be useful for library authors, my main concern is space for abuse because it should rarely be used in application codebases. Let's see. 👀

Btw, it's possible to use symbols for state/computed/method names. In the case of withEntitiesLocalSort:

const SORT_CONFIG = Symbol('SORT_CONFIG');

export function withEntitiesLocalSort<Input extends SignalStoreFeatureResult>(/* ... */) {
  return signalStoreFeature(
    type<Input>(),
    withMethods((store) => ({
      [SORT_CONFIG]: () => configFactory(store),
    })),
    // ...
  );
}

@gabrielguerrero
Copy link
Contributor

Hey @markostanimirovic, that withProps sounds super interesting :), you know, sometimes I think an experimental subpackage could be useful to get feedback from new ideas. And thanks I did not knew symbol props were working now thats great I will try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants