angular-architects / ngrx-toolkit

Various Extensions for the NgRx Signal Store
MIT License
177 stars 23 forks source link

suggestion/question: `withDataService` support for Observables #85

Open michael-small opened 3 months ago

michael-small commented 3 months ago

My problem

withDataService is super smart and interesting. Honestly the most boilerplate reducing Angular code I have seen. One limitation for my own use, however, is that I use rxMethod rather than promise.

My suggestions/questions

I copied over the relevant files of this library's withDataService into a private project full of more signal store experimentation I adapted all the feature methods to use rxMethod. I have not tested it too much yet, but it seems to work good so far. I can make it public if anyone is interested.

  1. If feasible, would there be interest in expanding withDataService to support RXJS?
    • I have an implementation working, but not yet with equivalent tests and not tested manually that much. If there is interest I would handle that, as well as documentation.
    • I certainly couldn't have written this from the ground up - I imagine that doing this would add a lot to the complexity.
  2. If there is not interest, how would I attribute this library properly so that I can use my variation under the MIT License
    • I may be overthinking things, but I haven't done something like this before: how should I go about fulfilling the whole MIT clause "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software."?
    • Include a markdown of it in the root of the feature?

edit: link to my implementation in one of my projects: https://github.com/michael-small/signal-store-playground/tree/main/src/app/store/withDataService-rxjs

rainerhahnekamp commented 3 months ago

@michael-small, I don't see a problem in supporting RxJs. So feel free to contribute your implementation

michael-small commented 3 months ago

Sounds good. I in fact do have some changes to make as I messed some stuff up, but once I have it in a better state I will update this issue and then get to a PR.

michael-small commented 3 months ago

Link to my implementation in a project: https://github.com/michael-small/signal-store-playground/tree/main/src/app/store/withDataService-rxjs

Once I have ironed it out a bit more I will make a fork of the toolkit and start adding an RXJS implementation.

michael-small commented 3 months ago

I have been grappling with types and generics when trying to adapt withDataService to also accept Observables. After trying and throwing away a lot of approaches, I think I found a decent one, and I was wondering if this sounds like a decent approach. The generic names are quite rough.

export interface DataService<
  E extends Entity,
  F extends Filter,
  ArrEntityData = Promise<E[]> | Observable<E[]>,
  SingleEntityData = Promise<E> | Observable<E>,
  VoidEntityData = Promise<void> | Observable<void>>
{
  load(filter: F): ArrEntityData;

  loadById(id: EntityId): SingleEntityData;

  create(entity: E): SingleEntityData;

  update(entity: E): SingleEntityData;

  updateAll(entity: E[]): ArrEntityData;

  delete(entity: E): VoidEntityData ;
}

Provided an interface like this one is agreeable, I still have to figure out how each withMethods method can handle it respectively in a nice way. But I thought it would be better to ask before I go too far with this approach.

Bottom line: naming aside, would you say that an interface like this is a good starting point?

rainerhahnekamp commented 3 months ago

@michael-small, what about that alternative Signature?

type PromiseOrObservable<Entity> = Promise<Entity> | Observable<Entity>;

export interface DataService<
  E extends Entity,
  F extends Filter
{
  load(filter: F): PromiseOrObservable<Entity[]>;

  loadById(id: EntityId): PromiseOrObservable<Entity>;

  create(entity: E): PromiseOrObservable<Entity>;

  update(entity: E): PromiseOrObservable<Entity>;

  updateAll(entity: E[]): PromiseOrObservable<Entity[]>;

  delete(entity: E): PromiseOrObservable<void> ;
}
michael-small commented 3 months ago

@rainerhahnekamp that's a lot better, thank you.

rainerhahnekamp commented 3 months ago

Great, waiting for your PR then 😃

michael-small commented 3 months ago

Thank you. Once I figure out how I can get each method to either narrow down what to do based on the type, or overload for the top level async or rxMethod signature, then I'll submit the PR.

Will you want tests for these? I don't see tests for the promises version, but I can try to make a test suite.

rainerhahnekamp commented 3 months ago

Yes please. Tests would be great

michael-small commented 3 months ago

@rainerhahnekamp I have a question: since rxMethod takes a static value, observable, or signal... what should be done if the implementing service is promise based and the inputs are an observable or signal?

I don't suppose we would make some rxMethod equivalent but for promises, but I also don't know how the utility would handle this scenario. Error message? Disclaimer to the user in the README.md and have that scenario just do nothing?

My approach right now is basically using type guards for telling if the implementing service method returns a promise or observable, so this is a particular edge case that I aught to handle before narrowing down other checks. So handle the logic based on the input first, and then presumably safely assume I use an rxMethod if the input is an observable or signal. The static value part and this question of mine is what is a bit tricky. Thoughts?

rainerhahnekamp commented 3 months ago

@michael-small I'd say for the first version, it should be fine of you check if the response is of type Promise or Observable. I guess you would do an automatic subscribe.

If there's demand, we can later maybe add the rxMethod and map a Promise to an Observable.

michael-small commented 3 months ago

Sounds good, thanks. It didn't occur to me that the RXJS calls could be an old fashioned subscription. I'm just so rxMethod minded.

I'll proceed with manual subscriptions. I assume in that case I can handle subscriptions with some type of self cleaning up subscription, or perhaps pass in a destroyRef. I'll go with the former to keep it simple.