urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.6k stars 448 forks source link

RFC: Add solid #3303

Open Nvos opened 1 year ago

Nvos commented 1 year ago

Summary

Add new package solid-urql providing solid-js bindings

Proposed Solution

Implement following hooks:

Suspense

Solution used in tanstack solid should be applicable for urql

Note

I'm willing to provide implementation, got already createQuery working along with some tests

kitten commented 1 year ago

An implementation already exists, see: #2527 I haven't actually tested and reviewed this myself yet, but it actually never got merged because tests were an issue and due to bandwidth constraints I wasn't able to actually look into this myself.

Also, see for prior discussion: https://github.com/urql-graphql/urql/discussions/3242#discussioncomment-6064034

I can't vouch for the correctness of #2527 entirely, but it had maintainer-oversight partially through it, so apart from tests it should be good to go, although I'd still have to take a look at reviewing it 🤔

So, if you don't mind, if we can get a testing approach combined with some of that previous code, we should get really close to being able to ship this 🙏

Nvos commented 1 year ago

Ok, thanks will check those out today

Nvos commented 1 year ago

I have reviewed #2527 and from what I see it levereges createResource to handle everyting. There's as well implementation provided in https://github.com/urql-graphql/urql/discussions/3242#discussioncomment-6071696 which is somewhat what I had in mind - it leverages createResource to trigger suspense.

From what I see there are few problems with implementation provided in #2527

createQuery

Problems

  1. It will always suspend which might not be desirable in all cases and implementing configurable suspense doesn't seem feasible
  2. Returned state is wrapped with Resource , ideally I think state should just be union of OperationResult and fetching. Wrapping it in Resource can be confusing as now we have error in Resource and after we get result we have error field in OperationResult as well. Additionally naming and structure of state is different than in other bindings, I think it would be great to have similar api to react
  3. Options are passed via single accessor

Proposal

Based off tanstack solid query createBaseQuery

  1. Persistent wonka subscription listening to makeSubject and using createResource only to trigger suspense, we could easily check provided suspense flag from client & context and react accordingly. Then any changes to variables or via refetch will just emit on subject
  2. Return union of OperationResult + fetching directly, via proxy in order to trigger suspense
  3. Provide each options as T | Accessor<T> - take a look at https://github.com/solidjs-community/solid-primitives/blob/main/packages/graphql/src/index.ts

createMutation

Problems

  1. Using createResource will suspend when executing mutation
  2. createResource primitive was meant for fetching data not executing mutations

Proposal

  1. Same thing as in other bindings - wonka stream which at specific moments updates local state and returns promise
  2. Provide each option as T | Accessor<T>
gksander commented 1 year ago

As author of #2527, I'll also apologize for never having the bandwidth to get that over the finish line. If you're interested in making Solid bindings a reality and have the bandwidth, it'd be amazing for you to take whatever is useful (if anything) from #2527 and join it with the approach from solid-query (which is realistically likely much better aligned with Solid best practices than #2527), and I'd be more than happy to review/help along the way!

I'd love to see solid bindings for urql, but have been pulled in other directions and just haven't had the pressing need to get solid bindings over the finish line.

Nvos commented 1 year ago

Thats great, will provide PR in upcoming week

Nvos commented 1 year ago

PR is up @gksander implementation is done, thought still got to add code docs, will work on them this week thought it should be mostly same as for react

kitten commented 1 year ago

Let me know if you'd like any work to be offloaded. I'll try to get around to a first pass review next week. Worst case the PR will get my full attention the week after, but I'm hoping we can get a prerelease ready before then. Thanks already so far for all the effort you've put in! 🙌

Nvos commented 1 year ago

No need to offload work, I'll have time this week I don't think that it should be much work as most of code docs will be same as for react with slight differences being:

ofarukaydin commented 1 year ago

Hey! Thank you so much for providing a cleaner implementation. As author of #3242 I'm looking forward to using this a drop in replacement of mine. I wonder if there is a cleaner way to integrate solid's built in hydration support with urql's ssr exchange. In my implementation I had to modify ssr exchange in order to make it work with solids built-in hydration. It was quite honestly more of a hack to get it working asap in production. With current status of the pr, one would need to serialize results to html in order to make ssr exchange work (this is how it works for official ssr exchange). But that means 2x serialization cost since solid already serializes promises and data itself too. In bigger queries paying the double cost of serialization is not optimal. Or maybe its out of scope for this PR?

Nvos commented 1 year ago

I don't know much about SSR in solid/urql, I'm fine to research and attempt to provide better SSR solution but I cannot say how long it will take.

Thought I think it could be possible to not store any data in createResource as in order to trigger and resolve suspense we need to:

  1. Execute resource read we do not need value only execute it in order to hook it to closes Suspense, this can be done in Proxy when we access data field. It matters where it is executed for example if we execute it inside createQuery then it will search for Suspense in wrapping components
  2. Possibility of resolving promise in createResource

For now I think it would be best to finish client side and provide SSR optimizations in separate PR. I can later on work on SSR but I'll need some time to research how to do SSR in solid/urql

kitten commented 1 year ago

I agree, let's focus on the main API. This is a very similar situation to @urql/next, which has recently changed to accommodate React Server Components — however much I think it's a little bit of a bad fit with GraphQL, it's a solution that allows for SSR in much of a similar way as Solid's resources being rehydrated on the client-side.

However, since rehydration involves a different set of data than what ultimately ends up in the resources, or rather, that comes out of createQuery, it's a question of implementing a similar mechanism as @urql/next to gradually prepare data for the ssrExchange.

All in all, that's stuff that I'd love to not have in a "v0.1" of @urql/solid. We can work on getting the first minor (pre-v1) ready, and then think of adding it either before v1 or after in a v1.1, i.e. a future feature release.

kaushalyap commented 11 months ago

@Nvos How is PR coming up?

Nvos commented 11 months ago

I have made tests more predictable last week, currently I'm trying to make tests run from root of repo (can run them from package implementing solid easily using additional vitest config) which seems to be problematic as it includes all packages with same config.

nikitavoloboev commented 7 months ago

@Nvos Can someone use this PR in a project?

I am trying to understand how to make use of this PR. Need graphql client in solid.js

Nvos commented 7 months ago

@nikitavoloboev Sure feel free to use it

From my perspective client is pretty much finished just couldn't get tests/lint to work with current state of urql repository thus from my side it is on hold for now. Bear in mind that it was only initially revevied by maintainers so there still might be problems with it.