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

suggestion/question: withDataService support for Observables #85

Open
michael-small opened this issue Aug 11, 2024 · 12 comments
Open

suggestion/question: withDataService support for Observables #85

michael-small opened this issue Aug 11, 2024 · 12 comments

Comments

@michael-small
Copy link
Contributor

michael-small commented Aug 11, 2024

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
Copy link
Collaborator

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

@michael-small
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

michael-small commented Aug 20, 2024

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
Copy link
Collaborator

rainerhahnekamp commented Aug 20, 2024

@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
Copy link
Contributor Author

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

@rainerhahnekamp
Copy link
Collaborator

Great, waiting for your PR then 😃

@michael-small
Copy link
Contributor Author

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
Copy link
Collaborator

Yes please. Tests would be great

@michael-small
Copy link
Contributor Author

michael-small commented Aug 22, 2024

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

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.

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

2 participants