query icon indicating copy to clipboard operation
query copied to clipboard

feat(angular-query): added observable support

Open rgolea opened this issue 1 year ago • 5 comments

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?

rgolea avatar Jan 17 '24 20:01 rgolea

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

vercel[bot] avatar Jan 17 '24 20:01 vercel[bot]

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 26de58096ad2a0a4e5bc600b666e33f8596ee7cb:

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

codesandbox-ci[bot] avatar Jan 17 '24 20:01 codesandbox-ci[bot]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 26de58096ad2a0a4e5bc600b666e33f8596ee7cb. 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.

nx-cloud[bot] avatar Jan 17 '24 20:01 nx-cloud[bot]

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.

arnoud-dv avatar Jan 18 '24 00:01 arnoud-dv

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!

rgolea avatar Jan 18 '24 02:01 rgolea

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 0.00%. Comparing base (3a127bf) to head (451c131). Report is 376 commits behind head on main.

:exclamation: Current head 451c131 differs from pull request most recent head 26de580

Please upload reports for the commit 26de580 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #6726       +/-   ##
==========================================
- Coverage   41.71%       0   -41.72%     
==========================================
  Files         179       0      -179     
  Lines        7007       0     -7007     
  Branches     1416       0     -1416     
==========================================
- Hits         2923       0     -2923     
+ Misses       3713       0     -3713     
+ Partials      371       0      -371     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 01 '24 15:06 codecov[bot]