search

jotaijs / jotai-tanstack-query

Jotai integration library for TanStack Query
MIT License
210 stars 14 forks source link

feat: (breaking) api overhaul #45

Closed kalijonn closed 10 months ago

kalijonn commented 1 year ago

I wanted to get your feedback on some of the changes. some tests are still breaking here.

My idea was to use as much from tanstack query as possible with very little implementation in jotai. With this, I've setup conditions which make suspense work only if suspense is true. tanstack query wants users to be explicit about suspense. Even in v5, they're creating new hooks (useSuspenseQuery) rather than making suspense default. This goes against jotai's default implementation? useAtom throws promise be default if it receives a promise.

Another one is refetch. some of the tests indicate that refetch triggers suspense but that's different from tanstack query. tanstack query does not trigger suspense when a query is refetching.

dai-shi commented 1 year ago

Even in v5, they're creating new hooks (useSuspenseQuery) rather than making suspense default.

In that case, the better abstraction might be separating atomWithQuery and atomWithSuspenseQuery.

dai-shi commented 1 year ago

And, it feels like the new jotai-tanstanck-query version should use v5, no?

kalijonn commented 1 year ago

Even in v5, they're creating new hooks (useSuspenseQuery) rather than making suspense default.

In that case, the better abstraction might be separating atomWithQuery and atomWithSuspenseQuery.

I agree.

And, it feels like the new jotai-tanstanck-query version should use v5, no?

they released a release candidate 2 days ago. I think the work I'm doing can work for both v4 and v5 with very little changes.

dai-shi commented 1 year ago

In that case, the better abstraction might be separating atomWithQuery and atomWithSuspenseQuery.

I agree.

Let's try that then.

they released a release candidate 2 days ago. I think the work I'm doing can work for both v4 and v5 with very little changes.

Do you want to continue supporting v4 for your new api? I think it can only be for v5.

codesandbox-ci[bot] commented 1 year ago

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 2da7b5687476f42f370f49d7a8abdd0dad7ad10a:

Sandbox Source
React Configuration
React TypeScript Configuration
kalijonn commented 1 year ago

I'm going the route of separating creating atomWithQuery and atomWithSuspenseQuery. will do the same for other *WithQueries as well.

atomWithQuery passes all tests. I've adjusted the tests to keep them inline with tanstack/query functionality.

created a new component QueryAtomErrorResetBoundary, to use in the place of QueryErrorResetBoundary from tanstack. tanstack's boundary uses context. I don't think we can use that directly within jotai atoms. tanstack-reference

kalijonn commented 10 months ago

@dai-shi everything except resetting error boundary support is done.

main changes:

  1. moved to v5
  2. separated suspense vs non-suspense

let me know what you think

dai-shi commented 10 months ago

Hi, thanks for working on this. I can't review everything, but looking at atomWithQuery and its related code, it generally looks good. Great work!

I would like to discuss one fundamental idea. Sorry, if I raise it too late. I don't remember which issues/discussions/chats we discussed, but someone suggested to use query cache as the source of truth. So, instead of creating query observer in each atom, we directly subscribe to query cache. How does that sound? We would like to consider if it's a viable solution.

kalijonn commented 10 months ago

I've looked into QueryCache and realized there's a lot of functionality that QueryObserver implements in terms of returing promises, conditionally fetching/pausing which we'll have to implement. We'd have to build our own QueryObserver.

While refactoring, I have tried to replicate the logic that tanstack hooks perform, in atoms. And in this process, I'm questioning that statement that 'query observer might not be a reliable source of truth'. I've been trying to add tests as issues come up. (most recent "on reset, atom wasn't suspending" - raised in discord)

For now, I haven't seen anything that points towards queryobserver not being reliable.

dai-shi commented 10 months ago

@kalijonn Can you join my Discord server and DM me? https://discord.gg/MrQdmzd

dai-shi commented 10 months ago

For now, I haven't seen anything that points towards queryobserver not being reliable.

Sounds good. If you considered it and the current conclusion is so, it's perfect.

cyrilchapon commented 10 months ago

@dai-shi I might install this on my side project; give it a shot and feedback. Any preferred channel ?

dai-shi commented 10 months ago

@dai-shi I might install this on my side project; give it a shot and feedback. Any preferred channel ?

That would be great! Please open a new issue in this repo for the feedback. You can install from the csb build. See "Local Install Instructions" in: https://ci.codesandbox.io/status/jotaijs/jotai-tanstack-query/pr/45

BTW, from now on, @kalijonn will be a new maintainer of this package.