jotaijs / jotai-urql

Jotai integration library for URQL
MIT License
29 stars 5 forks source link

breaking: make urql atoms behave as close to urql-react bindings as possible #11

Closed RIP21 closed 1 year ago

RIP21 commented 1 year ago

Changelog:

Why?

Current bindings are extremely far from being correct. I don't know how widely it's used and if used in production. There is no way to reexecute queries (there is no such way as refetching in URQL, only reexecute of operation with different context), the current refreshonly refreshes if the original context has requestPolicy being network-only or cache-and-network which is not how it suppose to be. It is also quite laggy causing hundreds of requests for some weird reason. These changes fix it all. Since they're using all the @urql/core APIs as correctly as I managed to understand (by looking through multiple exchanges, original react bindings source code, etc.), it works perfectly in tandem with original react bindings of URQL or independently. Cache and everything get reused, refreshed, etc. no matter what binding causes changes Jotai or URQL.

Two things that I didn't managed to align with URQL official binding.

I need help from @dai-shi to figure out how to suspend it during that recovery refetch (@kitten may hint smth) and, also, possibly, from both of you, to figure out why stale status is not getting reported even though the source reports that to the subscriber. It's important to notice, that suspense:true/false on urql client doesn't change either of these behaviors. Also, @kitten, will be glad if you will review refetching and other logic related to urql, as I feel that some of it is a bit redundant.

@dai-shi If merged I'll gladly update docs with a migration guide and examples.

Closes: #10

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 c39c426f4ccf8530699fbd146a38214f85ddedaf:

Sandbox Source
React Configuration
React Typescript Configuration
dai-shi commented 1 year ago

Wow, your work looks great. We have been looking for a maintainer for jotai-urql and you should be the one.

RIP21 commented 1 year ago

So @dai-shi yesterday I added an API that uses suspenseAtom that will allow users to rehydrate atoms with suspense false that will add an initialValue/Data into the atomWithObserver that effectively disables suspense for such users.

Also, I went ahead and removed the data atom. Also, I removed Action altogether. My logic behind it is that changes are already breaking enough, so trying to keep some backward compatibility is rather makes no sense at this point.

I also changed the API of all the atoms not to be multiple arguments, but a single object argument with multiple fields as it's easier to extend.

One thing I would wish to ship one day is getPause option that will allow for pausing of the query the same way as urql original client does. But I think it's rather a nice addition than what's absolutely needed now so I would postpone that to the next work.

RIP21 commented 1 year ago

So @dai-shi I've added getPause functionality that is almost 1:1 to URQL bindings (slightly different as it's impossible to trigger the fetch while paused, yet possible to work around by unpausing the query, which is IMO more expected behavior)

I also improved typings with some powerful generic types so now only needed and required types are allowed. I also added tests and type-tests for all these things.

I think it's ready to be shipped. Please, review, comment, and merge if you can. Commit history is pretty clean, so up to you if it makes sense to squash it.

Let me know where I can contribute to the docs. Also, I'll be happy with some sort of contributor role, so I can trigger workflows and whatnot. Thanks!

RIP21 commented 1 year ago

@dai-shi I need a bit of help from you.

For some reason when I make getVariables to be async function. And it makes all the atoms async as we want to await for them it in source atom to then chain all the way to the last one public to the user. But for some reason it starts to fail with r is not a function. Any clues?

I would want to ship this version and make it as a sepearted PR, so it's a bit clearer what I'm talking about, but if you can help in this one, then I'll be super happy to ship it with the support for async getVariables.

dai-shi commented 1 year ago

Also, I'll be happy with some sort of contributor role

Sent an invitation!

dai-shi commented 1 year ago

For some reason when I make getVariables to be async function. And it makes all the atoms async as we want to await for them it in source atom to then chain all the way to the last one public to the user. But for some reason it starts to fail with r is not a function. Any clues?

Hmm, no. We need to know what is r by disabling minification. (we should disable minification anyways. https://github.com/dai-shi/proxy-memoize/pull/66 )

I would want to ship this version and make it as a sepearted PR, so it's a bit clearer what I'm talking about, but if you can help in this one, then I'll be super happy to ship it with the support for async getVariables.

I don't think we could/should support async getVariables. In jotai-tanstack-query, we made a new wrapper function for async. Issue: https://github.com/jotaijs/jotai-tanstack-query/issues/21 PR: https://github.com/jotaijs/jotai-tanstack-query/pull/30

RIP21 commented 1 year ago

Thanks for comments. Will address most. Will make separate workflow etc. Will look into tanstack query thingy you mentioned.

RIP21 commented 1 year ago

So, I fixed:

dai-shi commented 1 year ago

Cool. Let me go the second iteration with fresh eyes.

RIP21 commented 1 year ago

@dai-shi updated everything as requested. Also added some comments here and there to clear things up. Also figured I don't need sourceAtom so inlined it into result atom.

dai-shi commented 1 year ago

@RIP21 Sounds good. Would you like to merge and release a minor bump? Or, do you want me to work on it this time?

RIP21 commented 1 year ago

@dai-shi unsure if it's a minor bump :) As APIs and other stuff got changed completely and is pretty breaking. So it's your call whether it's a minor bump or not. But for me, it sounds like a major bump :)

And yeah, if you can, please release it. As I don't have time now to figure out how it all (publishing in this project) works :)

dai-shi commented 1 year ago

@RIP21

This hasn't reached v1 yet. We add breaking changes in v0.x.

I'll merge it and make v0.7.0 release. Just adding a tag should trigger the CD workflow.

RIP21 commented 1 year ago

Gotcha. Ok then. Thanks :) I would need to update the docs for URQL Jotai.

Also, Jotai docs seem to be outdated and are still more about v1 than v2. Any plans to update that?

dai-shi commented 1 year ago

I would need to update the docs for URQL Jotai.

Thanks. Please open a PR there.

Also, Jotai docs seem to be outdated and are still more about v1 than v2. Any plans to update that?

We look for contributors. I think I updated at least, but there should be more fixes and improvements. You are more than welcome to help on it.

dai-shi commented 1 year ago

Published: https://www.npmjs.com/package/jotai-urql/v/0.7.0

(There were some errors of mine in cd.yml which is fixed in the main branch.)

sandhose commented 1 year ago

Just a heads-up that the published v0.7.0 has the typescript types in the wrong directory. They are in dist/src/src/index.d.ts, while the package.json references dist/src/index.d.ts

dai-shi commented 1 year ago

(Ahh, we don't have tests to cover that.)

@sandhose Do you know how to fix? dist/src/index.d.ts should be something we want.

dai-shi commented 1 year ago

Or, maybe we can fix package.json with dist/src/src/index.d.ts for now if it's easier.

sandhose commented 1 year ago

@dai-shi I think I managed to fix it in #12