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

feat(svelte): allow to pass a client directly to query, ... #1527

Closed

Conversation

sastan
Copy link

@sastan sastan commented Apr 8, 2021

Summary

This change supports the use of the svelte bindings outside of a component.

For example with @sveltejs/kit it is possible to use the load method to get data before the component is initialized:

<script context="module">
  import { get } from 'svelte/store';
  import { operationStore, query } from '@urql/svelte';

  export async function load({ context: { client } }) {
    const todos = query(
      operationStore(`
      query {
        todos {
          id
          title
        }
      }
    `),
      context.client
    );

    // Load the todos and make them available during initial rendering
    await todos.toPromise();

    return { props: { todos } };
  }
</script>

<script>
  export const todos

  // No need to invoke query(result)
</script>

Set of changes

@urql/svelte: adding client as last optional argument to query, subscription and mutation (not a breaking change – API compatible to previous versions)

@changeset-bot
Copy link

changeset-bot bot commented Apr 8, 2021

🦋 Changeset detected

Latest commit: 582321b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@urql/svelte Minor

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

@sastan sastan force-pushed the svelte-allow-to-pass-in-client branch from c5394a4 to 1967b4d Compare April 8, 2021 17:00
…on` and `mutation`.

This change supports to use the svelte bindings outside of a component.
@sastan sastan force-pushed the svelte-allow-to-pass-in-client branch from 1967b4d to 582321b Compare April 8, 2021 17:01
@kitten
Copy link
Member

kitten commented Apr 8, 2021

This change has been proposed before, but we don't want to support this explicitly because there are two other ways to cover this. The first method is to initialise the storage and query and use pause to trigger the query or to change an input: https://formidable.com/open-source/urql/docs/basics/svelte/#pausing-queries

The second, which is what you're trying to address are imperative calls. Imperative one-off calls are discouraged for queries because they'll be unable to update their data, i.e. listen to changes, but can already be done using client.query(...).toPromise().

So this inclusion hasn't been made because it'd encourage users to use @urql/svelte in such a way that makes updates impossible.

@sastan
Copy link
Author

sastan commented Apr 8, 2021

The first method is to initialise the storage and query and use pause to trigger the query or to change an input: https://formidable.com/open-source/urql/docs/basics/svelte/#pausing-queries

I do not want to pause the query. I want it to be executed right away and I need a promise to know when the data has been loaded. That way when the component renders initially it already has the data which allows SSR with sveltekit to work.

The second, which is what you're trying to address are imperative calls. Imperative one-off calls are discouraged for queries because they'll be unable to update their data, i.e. listen to changes, but can already be done using client.query(...).toPromise().

I only want the data to be available during initial rendering. After that normal bindings are used to trigger any updates.

So this inclusion hasn't been made because it'd encourage users to use @urql/svelte in such a way that makes updates impossible.

What would be the recommended way for async load/prefetch for sveltekit?

Edit: My current hack looks like this:

<script context="module">
  import { get } from 'svelte/store'
  import { operationStore, query } from '@urql/svelte'

  import * as gql from './queries'

  export async function load({ page, context }) {
    const store = operationStore(gql.QueryDocument, queryToVariables(page.query))

    const result = await client
      .query(store.query, store.variables, store.context)
      .toPromise()

    return { props: { result:  Object.assign(get(store), result) } }
</script>

<script>
  export let result
  query(result)

  // normal use – with initial data already available
</script>

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Apr 10, 2021

I wonder if this introduces a pitfall, I'm not very familiar with how SvelteKit works so forgive me if this assumption is wrong.

That type="module" script is executed on the server but injects the todos used in your example, what if someone does a mutation that invalidates the todos, now we don't have an observable stream on the client so the todos will remain stale. When we have an operationStore we have an open line of communication with the client so we can react to these changes.

Or is type="module" present on the client? Still we're doing toPromise() which means we're only taking the first result, this means that if we're using a cache that allows for partial results we can possibly only have a "half-response" on the first render which implies erroneous behavior.

Your workaround looks similar to how stuff is done in getServerSideProps, I know this feels contrived as you need to gather all queries up at the root but it kind off serves as an automatic performance improvement as well as you actively avoid introducing waterfalls, which sapper used to do as well. I guess that makes your workaround a valid way of doing this as it establishes the stream and hydrates the cache from the ssrExchange. I've also wondered about easier ways of achieving this in other frameworks, Vue and React handle this with Suspense as it's a pretty reliable way to know when the server can return an initial http-response, I've felt this is one of the few ways of incorporating initial data in the virtual dom.

@sastan
Copy link
Author

sastan commented Apr 10, 2021

<script context="module">: Marks the script block as module level – executed only once, eg top level module code. The normal script block is used as the component initializer and runs for each component instance.

load: A function that runs on the server and on the client to prefetch the data needed by a component instance before the instance is created.

toPromise(): That was/is a workaround for me to ensure the data is loaded before the store is passed to the component instance. All I really need is a way to know when the data has been fetched.

partial results: Not relevant for me at the moment. But would it be possible to wait until the data is not stale anymore?

mutation: That is why I want to have an observable stream. During SSR I do not care. But on the client the result should be updated. That is why I try to use the query binding.

Suspense: That would be great but not available yet. The current best practice is using the load function for prefetching data. If I could re-use the store that wouldn't add too much extra code/logic.

Below is an outline of how I would like to use urql with load:

<script context="module">
  import { operationStore, query } from '@urql/svelte'

  import * as gql from './queries'

  export async function load({ page, context }) {
    const result = query(operationStore(gql.QueryDocument, queryToVariables(page.query)), context.client)

    // TODO await until the store has fetched the data; maybe wait until the full response is fetched (not stale)
    // THIS IS JUST A PLACEHOLDER
    await result.ready()

    return { props: { result } }
</script>

<script>
  export let result

  // normal use – with initial data already available
</script>

@sastan
Copy link
Author

sastan commented Apr 10, 2021

Maybe the await result.ready() could be implemented like this:

function prefetch(store) {
  let unsubscribe
  return new Promise((resolve, reject) => {
    unsubscribe = store.subscribe((result) => {
      if (result.fetching) return
      if (result.stale) return

      if (result.error) {
        reject(result.error)
      } else {
        resolve(store)
      }
    })
  }).finally(() => unsubscribe?.())
}

This function could be part of @svelte/urql or in user land.

<script context="module">
  import { operationStore, query } from '@urql/svelte'

  import * as gql from './queries'

  export async function load({ page, context }) {
    const result = query(operationStore(gql.QueryDocument, queryToVariables(page.query)), context.client)

    // await until the store has fetched the data
    await  prefetch(result)

    return { props: { result } }
</script>

<script>
  export let result

  // normal use – with initial data already available
</script>

@kitten
Copy link
Member

kitten commented Apr 10, 2021

Hm, I so enjoy the idea of a native prefetch function in @urql/svelte. It's definitely a better fit than for other bindings where the promise call is more useful. I think this could be a good candidate for justifying #1515 further, since it'd make such operations and calls time-independent, meaning there wouldn't be a risk of accidentally introducing a race condition or of over fetching.

What I'm struggling with is the lack of "app-wide" abstractions for SSR. Admittedly, this is shaky in React as well and only Vue 3 has a solid concept of making such operations baked into the "normal" lifecycle of apps so far.

Just, so we get this right, let me try to reiterate this. The load method hands over data to a component and the component mounts using this data, mounting as usual otherwise? And this pattern in SvelteKit is isomorphic and runs in any environment, meaning that we're simply introducing a "first render" app-wide loading state?

I think prefetch could totally work in that case. I'd be curious about more semantics of client-side rehydration. I'd assume it wouldn't be different to other patterns in that the ssrExchange could take care of rehydration.

@sastan
Copy link
Author

sastan commented Apr 10, 2021

What I'm struggling with is the lack of "app-wide" abstractions for SSR. Admittedly, this is shaky in React as well and only Vue 3 has a solid concept of making such operations baked into the "normal" lifecycle of apps so far.

I hear you.

The load method hands over data to a component and the component mounts using this data, mounting as usual otherwise? And this pattern in SvelteKit is isomorphic and runs in any environment, meaning that we're simply introducing a "first render" app-wide loading state?

Yes! It is used on the server and the client.

I think prefetch could totally work in that case. I'd be curious about more semantics of client-side rehydration. I'd assume it wouldn't be different to other patterns in that the ssrExchange could take care of rehydration.

The ssrExchange is not needed for sveltekit. Sveltekit provides a fetch method (same API as global fetch, available on server and client) to load. I use that to create the urql client. On the server sveltekit tracks the requests made through fetch. On first render (re-hydration) on the client sveltekit re-uses the previous/server fetch result.

@sastan
Copy link
Author

sastan commented Apr 10, 2021

After looking more carefully at the query code I now better understand what the problem is:

function query(store) {
  // This subscription keeps the store updated
  const subscription = pipe(
    toSource(store),
    // ...
  );

  // Cleanup on unmount
  onDestroy(subscription.unsubscribe);
  return store;
}

My proposed solution would not work as there is not a current component (required by onDestroy) during the execution of load.

Maybe my hack (#1527 (comment)) is the way to go for now?

Have you ever considered making the subscription lazy/on-demand? Start/Stop the subscription based on subscribers to the store?

Something like this keeps could work:

import { readable } from 'svelte/store'

export function query(store, client = getClient()) {
  const { subscribe } = readable(store, (set) => {
    // Called when the number of subscribers goes from zero to one (but not from one to two, etc)
    const subscription = pipe(
      toSource(store),
      // ...
      subscribe(update => {
        _markStoreUpdate(update);
        store.set(update);
        // Just for now – there are surely better ways
        set(store);
      })
    );

    // Cleanup when there are no more subscribers
    return subscription.unsubscribe
  })

  // Just for now – there are surely better ways
  return Object.create(store, {
    subscribe: { value: subscribe }
  });
}

@kitten
Copy link
Member

kitten commented Apr 10, 2021

Have you ever considered making the subscription lazy/on-demand? Start/Stop the subscription based on subscribers to the store?

That's actually exactly how it works; that's just in the Core Client and not in any of the bindings packages.

Like I said, it's not hard to make this work. We'd start a source on the client and attach it to a promise (just like client.query().toPromise but instead we can write the result to the store directly.
I'd love to then add some filtering to ignore stale results given the purpose.

The PR I mentioned can also help here since it'd then not even matter whether the prefetch is started before, after, or while the component is mounted.

@sastan
Copy link
Author

sastan commented Apr 11, 2021

I think I have a better understanding of the bindings package now. Please correct me if I'm wrong here:

query: creates a subscription for the whole component lifecycle (init: subscribe, destroy: unsubscribe) which updates the store to support store.data and others without the need to use reactive prefix $store.data

As I understand my proposed change would therefore not work as the use needs to call query within the component script block for the subscription to get started.

Side note: I would prefer not to have the non-reactive bindings because the reactive prefix $ makes clear the you are using a store. But I know that would be a breaking change and maybe not what you would want for the library.

Maybe it would be possible to add an auto-subscription method . This wouldn't support the non-reactive prefix access – one must use $store.data and $store.variables = ....

import { readable } from 'svelte/store'

// The name is just a placeholder
export function autoquery(store, client = getClient()): Writable<OperationStore>  {
  const { subscribe }= readable(store, (set) => {
    // Called when the number of subscribers goes from zero to one (but not from one to two, etc)
    // Shared code with query could be re-factore into an helper method
    const subscription = sharedCodeFromQuery(client)

    // Propagate changes
    const unsubscribe = store.subscribe(set)

    // Cleanup when there are no more subscribers
    return () => {
     subscription.unsubscribe()
     unsubscribe()
   }
  })

  // Maybe expose some additional properties
  return {
   set: store.set,
   update: store.update,
   subscribe,
  }
}

export function prefetch(store: OperationStore): Promise<OperationStore> {
  let unsubscribe
  return new Promise((resolve, reject) => {
    unsubscribe = store.subscribe((result) => {
      if (result.fetching) return
      if (result.stale) return

      if (result.error) {
        reject(result.error)
      } else {
        resolve(store)
      }
    })
  }).finally(() => unsubscribe?.())
}

This would allow my use case like this:

<script context="module">
  import { operationStore, autoquery, prefetch } from '@urql/svelte'

  import * as gql from './queries'

  export async function load({ page, context }) {
    const todos = autoquery(operationStore(gql.TodosDocument, queryToVariables(page.query)), context.client)

    // await until the store has fetched the data
    await  prefetch(result)

    return { props: { todos } }
</script>

<script>
  export let todos

  // initial data already available
  // only reactive binding is supported
</script>

{#if $todos.fetching}
<p>Loading...</p>
{:else if $todos.error}
<p>Oh no... {$todos.error.message}</p>
{:else}
<ul>
  {#each $todos.data.todos as todo}
  <li>{todo.title}</li>
  {/each}
</ul>
{/if}

@sastan
Copy link
Author

sastan commented Apr 11, 2021

Great!

  • Do you need any help?
  • What can I do to support you?
  • Should I update this PR with to add autoquery and autosubscription? Or only the prefetchhelper?

@kitten
Copy link
Member

kitten commented Apr 11, 2021

Re: the reactive binding bit; that's actually necessary. We want to preserve state (hence the store is created once) and we want to observe changes to it, so the query is like a "directive" that watches the store for its inputs and updates the outputs. Hence it doesn't need to be reactive (although in some circumstances it could be part of a reactive statement)

I'll try to find some time to write an RFC to outline the proposed prefetch function so that the implementation path is clear and we document the decision. Once that's done if you'd like to pick it up, feel free to 💙 We generally make these so past decisions are transparent and future implementations have requirements to go by if needed.

That said, that means I'll close this PR for now. But until then we can keep using this thread to keep up the discussion until the RFC is ready ✌️

I'll try to get to the RFC this week, hopefully early this week. Even if you decide not to pick up the implementation, I'll post it here so you can take a look! Cheers 🙌

@kitten kitten closed this Apr 11, 2021
@sastan
Copy link
Author

sastan commented Apr 12, 2021

Sounds good! Just for clearifacation:

The query method is currently responsible for several things:

  1. get the client from the context – only possible within component initializer
  2. subscribe to the operation store
  3. update the store properties – to make store.data work; no $ prefix hence non-reactive
  4. unsubscribe on destroy – only possible within component initializer

The problem I'm currently trying to solve is that 1 and 4 are not possible within sveltekit load method. And 3 (store.data instead $store.data) is not needed or I would even say discouraged in our code base as it hides the reactivity of that statement.

What I was proposing is another method that does not support store.data but only $store.data. Basically only subscribe to the operation store using passed in client once there is a subscriber. This would require to use $ prefix on each store access. Svelte would take care of subscribing and unsubscribing for us.

The query method would use that method:

import { readable } from 'svelte/store'

// The name is just a placeholder
// `client = getClient()`: to support use within a component initializer -> `autoquery(store)`
export function autoquery(store, client = getClient()): Writable<OperationStore>  {
  const { subscribe }= readable(store, (set) => {
    // Called when the number of subscribers goes from zero to one (but not from one to two, etc)
    const subscription = pipe(
      toSource(store),
      // ....
      subscribe(update => {
        _markStoreUpdate(update);
        store.set(update);
      })
    )

    // Propagate changes
    const unsubscribe = store.subscribe(set)

    // Cleanup when there are no more subscribers
    return () => {
      subscription.unsubscribe()
      unsubscribe()
    }
  })

  // Maybe expose some additional properties
  return {
   set: store.set,
   update: store.update,
   subscribe,
  }
}

// Only handles component lifecycle stuff
export function query(store): Writable<OperationStore>  {
  // Activate component subscriber to update non-reactive store properties
  const unsubscribe = autoquery(store, getClient()).subscribe(noop)
  onDestroy(unsubscribe);
  return store;
}

If I understand you correctly the RFC would ensure that all subscribers get the same data, right?

Would something like autoquery (name is up for debate) be considered for inclusion? Together with prefetch that would solve my use case very nicely.

@kitten
Copy link
Member

kitten commented Apr 12, 2021

Actually, you do need to write things like $store.data to actually subscribe to them under the hood, but what I meant was that query doesn't need to be a reactive statement or inside one.

I'm not quite sure what you're trying to show with autoquery tbh, because it'd explicitly be a preload in my mind. Also there'd be no subscription. The purpose of this function would be to return a promise & load another piece of data just once.

@sastan
Copy link
Author

sastan commented Apr 12, 2021

My apologies for not being clear enough.

Actually, you do need to write things like $store.data to actually subscribe to them under the hood, but what I meant was that query doesn't need to be a reactive statement or inside one.

That is my point and the difference between query and autoquery. I want to use something like query but create/initialize/use it outside of a component and I may want to enforce the use of reactive prefix to access the store properties.

On a personal note: I think the property shortcut access brings more harm than it may be useful because non-svelte reactive prefix access kinda works but is not updated. Enforcing the use of $ prefix would prevent these kinds of bugs. But that is just a personal preference.

I'm not quite sure what you're trying to show with autoquery tbh, because it'd explicitly be a preload in my mind. Also there'd be no subscription.

The autoquery would allow to use the query subscription logic outside of a component. The urql subscription would be managed by svelte store if at least one svelte subscriber is subscribed (see #1527 (comment)).

The purpose of this function would be to return a promise & load another piece of data just once.

That is what prefetch is for.

  • query – must be called in the component initializer and data can be accessed without svelte reactive prefix, but prefix is needed for svelte to pickup changes

    <script>
    let todos = query(operationStore(...))
    // works kinda – but no updates
    todos.data
    // must use reactive prefix to get updates
    $todo.data
    </script>
  • autoquery – may be called with a client (eg outside of a component initializer), manages its urql subscription based on how many active svelte subscribers, and requires a svelte reactive prefix

    For my main use:

    <script context="module">
    export async function load({ context }) {
      return { props: { todos: await prefetch(autoquery(operationStore(...), context.client)) } }
    }
    </script>
    
    <script>
    export let todos
    // No need to call `query` here
    // But must be used with svelte reactive prefix to allow managing its underlying urql subscription
    // Svelte will cleanup for us on destroy and if there are no more svelte subscribers the underlying urql subscription will be removed
    $todos.data
    </script>

    But this would work as well:

    <script>
    const todos = autoquery(operationStore(...))
    // must use reactive prefix to access data
    $todo.data
    </script>

    I know the name autoquery is not the best. Maybe query$?

  • prefetch – is responsible for preloading that data (omitted in the example above because it is the same in feat(svelte): allow to pass a client directly to query, ... #1527 (comment))

I think this can be implemented by extracting the non-component stuff from query (extracting it) into an own method (autoquery) to make that functionality re-usable. query would then be the light wrapper shown above that handles the component specific stuff.

@jycouet
Copy link

jycouet commented Apr 14, 2021

I would love to be as advanced as you @sastan 👍
I'm trying to have urql working with sveltekit, but I'm not able to. Would you have an example somewhere so that I can dig into it?
//Thx

@sastan
Copy link
Author

sastan commented Apr 14, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants