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(angular-query): added observable support #6726

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rgolea
Copy link
Contributor

@rgolea rgolea commented Jan 17, 2024

I've added a query$ property on the options of the query. It's a function that accepts a context just like the queryFn prop does and it returns an observable. It automatically unsubscribe to the observable if the abort signal is called.

@arnoud-dv how does this look?

Copy link

vercel bot commented Jan 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
query ⬜️ Ignored (Inspect) Visit Preview Jan 17, 2024 10:21pm

@rgolea rgolea marked this pull request as draft January 17, 2024 20:49
Copy link

codesandbox-ci bot commented Jan 17, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 26de580:

Sandbox Source
@tanstack/query-example-angular-basic Configuration
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

@rgolea rgolea force-pushed the feature/angular-observable-support branch from 53df354 to 3cca2ec Compare January 17, 2024 20:52
Copy link

nx-cloud bot commented Jan 17, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 26de580. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@rgolea rgolea force-pushed the feature/angular-observable-support branch from 3cca2ec to 38ca649 Compare January 17, 2024 20:56
@rgolea rgolea marked this pull request as ready for review January 17, 2024 20:58
@rgolea rgolea force-pushed the feature/angular-observable-support branch from 38ca649 to 451c131 Compare January 17, 2024 22:18
chore(angular-query): fix knip issues

feat(angular-query): throw errors when the observable breaks
@rgolea rgolea force-pushed the feature/angular-observable-support branch from 451c131 to 26de580 Compare January 17, 2024 22:21
@arnoud-dv arnoud-dv self-assigned this Jan 17, 2024
@arnoud-dv
Copy link
Collaborator

First thoughts:

Can we do this via an overload on queryFn? In this implementation it is possible to pass a function to queryFn and a function returning an observable to query$ at the same time. This isn't very intuitive IMO.

I'm not convinced we should accept values coming in from the observable after the first one. This behaviour is quite different from the other adapters where a value is received once. Didn't delve deep into this yet but I can imagine APIs aren't designed for this and a few bugs may result from this too. It also needs quite a lot of code (bundle size) for something that won't be used very much. We may choose to support it but I don't think injectQuery would be the place, maybe something like injectStreamingQuery and we may need changes in core for that. What do you think?

If we could simplify this a bit with an overload and support a single value emission only it will be a great addition.

@rgolea
Copy link
Contributor Author

rgolea commented Jan 18, 2024

Maybe we can rename it to observableFactory. However, I did try to return the type of Observable on queryFn but there are 2 issues I came across with:

  • The defaultQueryOptions function starts to break apart as it doesn't know it can accept an Observable and I can't really override that. Besides, there are lots of types that start breaking (including the query-options file) and I'm not really sure that rewriting all those types is worth the effort. It would have been if we could've supported it on the core package but it's out of scope at the moment.
  • In order to check for the proper return type of the queryFn, in case it supported an Observable, I would've had to execute the function in the computed signal that creates the defaultedOptionsSignal and I believe that would be innecessary. I would rather let the client know it should perform the queries.

On the injectStreamingQuery part of the discussion, I believe having an Observable as an option allows you to control what flow you decide to pass down to the queryClient. Observables, by definition can emit multiple times and I believe it would be a really bad DX to allow something to behave contrary to it's natural state.

That being said, we could try and just support observables only on the new inject function but I believe it's name would be confusing to say the least. However, given Angular's normal behaviour, the http client should just return a response once and it should just finalize. That's why I'm thinking this new function wouldn't bring too much value.

Can't wait for feedback on all this. Thanks!

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.

None yet

2 participants