-
-
Notifications
You must be signed in to change notification settings - Fork 454
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
(svelte) - Reimplement @urql/svelte bindings with new API approach #1016
Conversation
🦋 Changeset detectedLatest commit: 3c76018 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Amazing! Can't wait to try it! Latest commit is broken. |
d906eea
to
b2d651e
Compare
Real-time reproduction with latest commit: https://codesandbox.io/s/urql-svelte-crud-with-new-bindings-rbrsb?file=/List.svelte. Pending issues:
|
@frederikhors Yep, I think there's a small timing issue still where some synchronous retries lead to our Svelte implementation becoming stuck. But once we fix that this will be ready to go 👍 Edit: Almost forgot; this is what I'm playing around with. But it can also replicate the issue https://codesandbox.io/s/urql-svelte-crud-pollinterval-keeps-calling-forked-lseuf?file=/urql.js |
326d51d
to
d540b0f
Compare
When GraphQLRequest is passed this is usually fine, but when the operation is then instantiated multiple times per component lifecycle, this causes a memory leak of multiple active subscriptions until the component unmounts. This can simply be discouraged by requiring the store.
d540b0f
to
c9fa6a6
Compare
It's looking pretty fine to me now; the Graphache timing fix for the Since this is looking very promising I'd like to take some time to write some docs sections for Svelte and ship this branch as the first stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome
This has just been published as |
Resolve #1007
This is intended to replace the current Svelte bindings as either a new breaking beta release, or as the stable v1 release.
The initial idea is to move towards an approach that fully encapsulates the
OperationResult
output andGraphQLRequest
input states into one mergedWritable
store. This store can be observed by components and by operations, e.g.query
andsubscription
.This effectively and intuitively steers users away from pitfalls and covers easy to miss edge cases, where our previous implementation would create memory leaks. This API is focused on providing a single way to do things; this is especially advantageous since
client.query
shortcut methods and the likes are still accessible.So overall this focuses on a single primitive to create a value container in Svelte that is only mutable in certain ways (only
query
,variables
, andcontext
can be changed). The other constraint is that operations likequery
andsubscription
should be called at most once, which is easy to teach and looks as imperative as most Svelte code does anyway.Another nice advantage of the
OperationStore
is that it's both the stateful object and a Svelte writable store in one. It may still be passed around, outside of Svelte components, and can be mutated and read, since it provides getters and setters and prevents changes to result properties to keep things safe.