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

RFC: Add solid #3303

Open
Nvos opened this issue Jul 2, 2023 · 15 comments · May be fixed by #3327
Open

RFC: Add solid #3303

Nvos opened this issue Jul 2, 2023 · 15 comments · May be fixed by #3327
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release

Comments

@Nvos
Copy link

Nvos commented Jul 2, 2023

Summary

Add new package solid-urql providing solid-js bindings

Proposed Solution

Implement following hooks:

  • createQuery
  • createMutation
  • createSubscription

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

@Nvos Nvos added the future 🔮 An enhancement or feature proposal that will be addressed after the next release label Jul 2, 2023
@kitten
Copy link
Member

kitten commented Jul 2, 2023

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: #3242 (comment)

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
Copy link
Author

Nvos commented Jul 3, 2023

Ok, thanks will check those out today

@Nvos
Copy link
Author

Nvos commented Jul 3, 2023

I have reviewed #2527 and from what I see it levereges createResource to handle everyting. There's as well implementation provided in #3242 (comment) 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
Copy link
Member

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
Copy link
Author

Nvos commented Jul 15, 2023

Thats great, will provide PR in upcoming week

@Nvos Nvos linked a pull request Jul 23, 2023 that will close this issue
@Nvos
Copy link
Author

Nvos commented Jul 23, 2023

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
Copy link
Member

kitten commented Jul 23, 2023

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
Copy link
Author

Nvos commented Jul 24, 2023

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:

  • Hook naming - different convention prefix is create instead of use
  • Some args are union of accessor and value

@ofarukaydin
Copy link

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
Copy link
Author

Nvos commented Jul 26, 2023

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
Copy link
Member

kitten commented Jul 26, 2023

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
Copy link

@Nvos How is PR coming up?

@Nvos
Copy link
Author

Nvos commented Oct 8, 2023

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
Copy link

@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
Copy link
Author

Nvos commented Jan 15, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants