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

feat: add support for observables to withDataService #88

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

michael-small
Copy link
Contributor

@michael-small michael-small commented Aug 27, 2024

This PR would close issue 85: suggestion/question: withDataService support for Observables

What was done

  • The interface for the implementing services that use withDataService was expanded to take Observable methods
    • type PromiseOrObservable<Entity> = Promise<Entity> | Observable<Entity>;
    • ex) loadById(id: EntityId): PromiseOrObservable<Entity>;
      • Existing functionality supports something like this
        • loadById(id: EntityId): Promise<Flight> {...}
      • These changes would now also support something like this
        • loadById(id: EntityId): Observable<Flight> {...}
  • The withDataService methods were adapted to take an observable (returns a subscription) as well as already taking promises
    • type PromiseOrSubscription<Entity> = Promise<Entity> | Subscription;
    • ex) [K in Collection as `load${Capitalize<K>}Entities`]: () => PromiseOrSubscription<void>;
      • The method checks if something is an instance of an observable. If so, the method manually pipes and subscribes. If not, the method resolves the call as a promise.

edit:
What was chosen not to be done, as per talk in the issue

  • For the moment, rxMethod is not used. The solution currently just manually subscribes. Supporting rxMethod would widen the feature to need to take non-static values such as signals or observables.
    • "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." - Rainer

What is left to be done (before turning from Draft PR to regular PR)

  • Tests for both the promises and observables
    • There are not already existing tests for withDataService as is with promises, so I should probably make those first and get those in as a PR before moving onto this one. Thoughts?
    • Observable tests should follow
    • I am admittedly not very experienced/good at testing, so testing is probably the main hurdle now that implementation is hopefully all good apart from iteration on feedback.
  • Explicitly handle unsubscribing
    • HTTP observables tend to be considered self handling observables if I understand correctly, but it would be good to unsubscribe anyways
    • Pass in an optional DestroyRef? Otherwise basic take(1) or first or something?

@michael-small
Copy link
Contributor Author

I forgot to actually push up the usage of the RXJS equivalent service and point that service at the right endpoint... fixed 47fc853

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

Successfully merging this pull request may close these issues.

1 participant