HoudiniGraphql / houdini

The disappearing GraphQL framework
http://www.houdinigraphql.com
MIT License
898 stars 95 forks source link

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

Open Tamxaun opened 2 years ago

Tamxaun commented 2 years ago

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 commented 2 years ago

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

AlecAivazis commented 2 years ago

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?

ghost commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

fehnomenal commented 2 years ago

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

AlecAivazis commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

lusid commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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.