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

Providing different variables for pagination instead of offset and limit #445

Open
Tamxaun opened this issue Aug 1, 2022 · 20 comments
Open
Labels
Enhancement A new feature or improvement to Houdini's public API

Comments

@Tamxaun
Copy link

Tamxaun commented Aug 1, 2022

I'm using KeystoneJS graphql as my backend and they schema look like this:

items(
  where: ItemWhereInput! = {}
  orderBy: [ItemOrderByInput!]! = []
  take: Int
  skip: Int! = 0
): [item!]

KeystoneJS using take and skip instead of offset and limit. Is there a way to support those variable names?

@Tamxaun
Copy link
Author

Tamxaun commented Aug 1, 2022

As a temporary solution, I extend schema and created a new query with different variable names.

@AlecAivazis
Copy link
Collaborator

Unfortunately, there isn't a way to do this yet. I'm been wrestling with what the API could look like for providing custom pagination implementations. Do you have any ideas?

@AlecAivazis AlecAivazis added the Enhancement A new feature or improvement to Houdini's public API label Aug 1, 2022
@ghost
Copy link

ghost commented Aug 28, 2022

This is the case with FaunaDB as well:
https://docs.fauna.com/fauna/current/learn/tutorials/graphql/pagination

type Query {
  allTemplates( _size: Int _cursor: String ): TemplatePage!
}

type TemplatePage {
  data:[Template]!
  after: String
  before: String
}

@ghost
Copy link

ghost commented Aug 28, 2022

Unfortunately, there isn't a way to do this yet. I'm been wrestling with what the API could look like for providing custom pagination implementations. Do you have any ideas?

@AlecAivazis
I'm thinking it could be done in the Houdini Config file ?

I'm open to do a PR if you agree.

/** @type {import('houdini').ConfigFile} */
const config = {
  //...
  pagination: {
    fields:{
      limit:{
        arg: 'take',
        type: 'Int'
      },
      offset:{
        arg: 'skip',
        type: 'Int'
      }
    }
  }
}

@jycouet
Copy link
Contributor

jycouet commented Aug 28, 2022

Today houdini supports 2 styles in a 'hard coded way'.

One option (A) is to support 3 or 4 ways in a 'hard coded way', another option (B) is to give users the ability to provide custom fields and logic. Another option (C) is to map style 3 to style 2 (what I think you suggest with the config update).

The best would be B as it gives the most freedom and houdini can provide n styles directly included.
The easiest is probably A, as it's "just" supporting new style, (but requires some code, integration, maintenance, doc... )
I'm not really in favor of C, because it's style using 'hard coded' + user config mapping things to things... (validation? What is mappable to what? Doc, ... )

Could you imagine starting a PR for A? Or what do you think?

@ghost
Copy link

ghost commented Aug 28, 2022

@jycouet

I'm still reading the source.
Great job by the way.

Since I want to be able to use Houdini's pagination features with fauna for work, I'm open to make a PR with my fork maybe later today, but the thing is I see this as a more of an easy fix.

I'm in favour of C as well, I'll see what I can do.

@ghost ghost mentioned this issue Aug 29, 2022
4 tasks
@fehnomenal
Copy link
Contributor

What about having fields on the @paginate that can be used to customize the names?

@AlecAivazis
Copy link
Collaborator

AlecAivazis commented Sep 6, 2022

I just realized I never got down my full thoughts on this. I think if our goal is to support custom approaches to pagination it would be helpful to figure out what are the various parts that would have to be customizable in order to support the two current options. That is to say, that the current pagination logic should exist as a built in version of whatever a user would do to specify their own. So with that in mind here is what we need to let the user specify:

  • How to detect the pagination strategy. atm, @paginate looks for a first: Int, after: String | Cursor for forwards cursor, last:Int, before: String | Cursor for backwards cursor, and offset: Int, limit: Int for offset/limit. This could be done explicitly with an argument to the @paginate directive like @fehnomenal suggested (with a default value set in the config file) but the current approach allows for both strategies to exist in the same project without any intervention on the user. Would be nice to keep that magic if possible. We also need to know which of these defines the pageSize.
  • How to compute the variables for the "next page". In forwards cursor-based based pagination you need to go look at the pageInfo. endCursor of the tagged field for the after variable. In offset, you need to count the number of elements in the list.
  • What do we do with pageInfo? Is it just an on or off thing? Another way to say that is do we force all pagination implementations to either fit the PageInfo mold or have no way of tracking the pagination state as a top level field on the store? Or do we have to generalize things so that users can provide their own fields in the store value?
  • What methods get added to a store? Atm, stores with cursor-based pagination get either loadNextPage or loadPreviousPage. I'd like to be able to say that all pagination implementations just get a loadNextPage and the forward/backwards distinction doesn't affect the API but unfortunately, we need to be able to support bi-directional cursor-based pagination even though its not currently supported. This means that a store could have both loadNextPage and loadPreviousPage at the same time.
  • How to insert the next page query result into the current list. This one is kind of subtle and is one of the big hurdles i've been trying to work through in my head. Basically the question is which values get pushed onto the current cached value when adding a page to the current list. For offset, this is easy: the field marked with @paginate gets pushed onto the current value. However, for cursor-based pagination the runtime needs to look at the edges.node of the marked field and prepend or append depending on the method used.
  • Is there a necessary sub selection for queries when loading an additional page?

I know this is probably a lot more than people were expecting but I think it would be good to get on the same page about what's needed in order to support custom pagination strategies. I think a field mapping like @DanielHritcu is a good idea but only works in the absolute simplest cases that are directly equivalent to what's already supported. This feature is 1000% necessary but very very hard to get right which is why I haven't really spent too much time on it.

This need for custom methods on the stores makes me think that we should let users be able to provide custom classes that we will use when instantiating a paginated store. That means that there are 2 parts of the API that we need to consider: the interfaces of classes themselves as well as the config value pointing to the custom class.

@ghost
Copy link

ghost commented Sep 12, 2022

Since the current @paginate directive uses hardcoded fields.
Also, the rewrite required to make a universal solution for all the cases, would require quite some time.

Would a middleware approach work better?
Provided there are some Houdini AST helpers of course.

My rough and maybe wrong idea would be to alter the document AST before passing it to the paginate or list directive and change it back when the client is built.

Don't know where along Houdini's transformer process the two functions will be inserted.
The alter, before the paginate transformer, but I have no clue, when the revert would kick in.

For more custom use cases, that go outside the already existing strategies ( cursor, offset), maybe provide a custom transformer to replace the existing one ?

Config could be something like:

// ...
/** @type {import('houdini').ConfigFile} */
const config = {
  client: './src/lib/houdini.ts',
  schema: './schema.graphql',
  transformers: {
    pagination: customPagination
  }
};

In case the already existing strategies can provide the functionality:

// names are chosen for the sake of the example.
export const paginationMiddleware = {
  alter: (ast: DocumentNode) => {
    return visit(ast, {
      // We look for `_size` and `_cursor` arguments and change them to 
      // `first`, `after` to fit Houdini's implementation.
      Argument(node) {
        switch (node.name.value) {
          case '_size':
            (node as any).name.value = 'first'
            return node
          case '_cursor':
            (node as any).name.value = 'after'
            return node
          default:
            break
        }
      },
      // ...
    })
  },
  revert: (ast: DocumentNode) => {
    return visit(ast, {
      Argument(node) {
        // Revert the argument names back.
        switch (node.name.value) {
          case 'first':
            (node as any).name.value = '_size'
            return node
          case 'after':
            (node as any).name.value = '_cursor'
            return node
          default:
            break
        }
      },
     // ...
    })
  },
  
  // Don't exactly know if the response needs to be remapped also.
  // Also if this would affect the generated types.
}

Config could be something like:

import {paginationMiddleware} from '@houdini/middleware/pagination-fauna'
/** @type {import('houdini').ConfigFile} */
const config = {
  client: './src/lib/houdini.ts',
  schema: './schema.graphql',
  middleware: {
    pagination: paginationMiddleware
  }
};

export default config;

At the moment I used something like this on my graphql yoga server to remap requests and responses from faunadDB specific to relay so Houdini works out of the box.

But this requires that I maintain a schema Houdini is compatible with, and also I need to perform the remapping on each request.

If I could perform this transformation at build time with Houdini, that would be amazing.

@AlecAivazis
Copy link
Collaborator

AlecAivazis commented Sep 29, 2022

Sorry I haven't replied here yet @DanielHritcu. Every time I read what you wrote, I get distracted with some thought process and end up writing a bunch of notes for what a Houdini plugin should look like. I think you're onto something big 👍

I've been working on fleshing this idea out a little bit for the last few days to figure out the best way to support multiple frameworks and i think what you described is falling very nicely into how I've been thinking about it. Mind if I ping you for feedback when I have something a little more concrete?

@AlecAivazis
Copy link
Collaborator

Okay so with 0.17.0 there is now an official, undocumented framework for building plugins to houdini. While the exact pieces necessary to support custom pagination aren't all there, the general approach would be to create a custom directive that you can use to specify a field uses your custom pagination implementation. While it's not perfectly integrated into the existing houdini directive, it simplifies the picture considerably since we don't have to hook into a very random part of the codebase.

If someone is interested in helping connect the remaining dots, I would gladly provide any assistance they required. probably good start with a voice chat to go over what's in place, what's still needed, etc.

@ghost
Copy link

ghost commented Nov 11, 2022

@AlecAivazis
Sorry for the late reply. I'm down for a chat regarding the pagination plugin.
Let me know how I can reach you.

@AlecAivazis
Copy link
Collaborator

No problem at all! Discord is best - you can find me on the svelte discord as @AlecAivazis

@lusid
Copy link

lusid commented Jan 12, 2023

Just to add another example to the mix, Postgraphile uses first/offset to handle its limit/offset strategy (there's no limit field because first accomplishes the same goal). So if Houdini just looks for first and assumes a forward cursor strategy is desired, it wouldn't allow for a limit/offset strategy, even if I were able to transform the name of limit to first.

@AlecAivazis
Copy link
Collaborator

Oh that's really good to keep in mind! We do try to be a little smarter than that and inspect the schema to see which strategy it satisfies (so for cursors, it looks for edges, pageInfo, etc.). We only use the fist argument to track the page size to compute future values of offset

@lusid
Copy link

lusid commented Jan 12, 2023

@AlecAivazis Sorry if I'm misunderstanding the last part of what you said, but are you implying there is a way currently to use Automatic Loading with the @paginate directive that supports using first/offset instead of limit/offset? The only way I've been able to do this so far is to remove @paginate entirely and use either 1) Manual Loading or 2) by using the _XVariables method in +page.js to pull out a page variable from url.searchParams for the page and then calculating the first/offset myself (which then requires me to manage separate page variables in searchParams).

Is there a better way to do this now that I'm just not gleaming from the docs?

For clarification, I'm doing this:

// +page.gql
query Info($first: Int = 10, $offset: Int = 0) {
  tests(first: $first, offset: $offset) {
    nodes {
      id
      name
    }
    pageInfo {
      hasPreviousPage
      hasNextPage
    }
  }
}
// +page.js
export function _InfoVariables({ url }) {
    const p = parseInt(url?.searchParams?.get('p')) || 1

    return {
        first: 10,
        offset: (p - 1) * 10
    }
}
// +page.svelte
<script>
	import { page } from '$app/stores';

	export let data

	$: ({ Info } = data)

	$: p = parseInt($page.url.searchParams?.get('p') || "1")
</script>

{#each $Info.data.tests.nodes as node}
<div>{node.name}</div>
{/each}

{#if $Info.data.tests.pageInfo.hasPreviousPage}
<a href="?p={p-1}">Prev</a>
{/if}

{#if $Info.data.tests.pageInfo.hasNextPage}
<a href="?p={p+1}">Next</a>
{/if}

This definitely seems like overkill, so if you have any other suggestions for how to do this now that are less verbose, I'm all ears.

@AlecAivazis
Copy link
Collaborator

No, not yet. There's no way yet to customize the pagination arguments. I just wanted to clarify that we wouldn't have to change the top level api of assigning first and using @paginate when we do come up with a way to extend the pagination behavior

@AlecAivazis
Copy link
Collaborator

AlecAivazis commented Jan 25, 2023

For your exact situation @lusid, I would probably move the query definition into the +page.js file so that its all colocated:

// +page.js
import { graphql } from '$houdini'

export const _houdini_load = graphql(`
    query Info($first: Int = 10, $offset: Int = 0) {
      tests(first: $first, offset: $offset) {
        nodes {
          id
          name
        }
        pageInfo {
          hasPreviousPage
          hasNextPage
        }
      }
    }
`)

export function _InfoVariables({ url }) {
    const p = parseInt(url?.searchParams?.get('p')) || 1

    return {
        first: 10,
        offset: (p - 1) * 10
    }
}

Also, it might be helpful to know that the store value has an attribute that holds the variables so you don't need to do parseInt($page.url.searchParams?.get('p') || "1") twice

@AlecAivazis
Copy link
Collaborator

A very rough note without any context for future alec that i don't want to forget:

we could allow users to add custom updates to the artifact that they can teach the cache how to perform. Atm, we support prepend and append for our pagination logic. maybe the user wants to define a special patch update that let's them do something crazy when applying a server response to the cached value. this could be used for custom pagination implementations or even to make situations look like pagination that don't conform to houdini's assumptions.

@jason-johnson
Copy link

Any updates on this one? I'm trying to test with GraphQL Zero and am not sure how to set up the paging without doing everything manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement A new feature or improvement to Houdini's public API
Projects
None yet
Development

No branches or pull requests

6 participants