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

Pagination and generics #529

Open
fezfez opened this issue Nov 24, 2022 · 5 comments
Open

Pagination and generics #529

fezfez opened this issue Nov 24, 2022 · 5 comments

Comments

@fezfez
Copy link
Contributor

fezfez commented Nov 24, 2022

Hello,

This issue is just to open a discussion.

I would like to write my paginated query like this :

class MyController
{
    /**
     * @return Porpaginas\Result<Product>
     */
    #[Query]
    public function products(): Porpaginas\Result
    {
        return new Porpaginas\Doctrine\ORM\ORMQueryResult($doctrineQuery);
    }
}

instead of :

class MyController
{
    /**
     * @return Product[]
     */
    #[Query]
    public function products(): Porpaginas\Result
    {
        return new Porpaginas\Doctrine\ORM\ORMQueryResult($doctrineQuery);
    }
}

It will better match with generic support in other tools in general (phpstan, psalm, phpstorm etc...) but the down stream (Porpaginas) does not actually support generics

Note : Support for generic already exist (#468)

@oojacoboo
Copy link
Collaborator

What is needed for support? Does Porpaginas need to implement an interface? What causes it to not support generics?

@oprypkhantc
Copy link
Contributor

@oojacoboo This is causing issues for us as well (for Laravel paginator, not Porpaginas). Laravel itself doesn't support generics everywhere, but larastan (extension of phpstan for Laravel) does define LengthAwarePaginator generic.

Right now graphqlite parses generic types incorrectly: LengthAwarePaginator<Product> is treated as a "list type" by BaseTypeMapper and hence wrapped in GraphQLType::listOf($innerType).

Generic types are just regular Object_ types with additional parameters, so they should be treated as such. Right now graphqlite assumes that generic types are lists and wraps them in GraphQL's list in BaseTypeMapper. It shouldn't do that; there are plenty of cases where a generic type is not a list, Paginator being one of them.

What I propose (keeping the backwards compat) is the following flow for generic types, in order:

  • when an object is Paginator (either Porpaginas or LengthAwarePaginator), map it manually
  • when an object is iterable, treat is as a list of type by default, keeping backwards compat with all the custom Collection and similar types
  • otherwise fallback to other mappers

Don't know on the implementation yet. Thoughts?

@oojacoboo
Copy link
Collaborator

oojacoboo commented Mar 12, 2023

I haven't used the pagination implementation yet, so I'm not familiar with it. That said, I think we need to support an interface implementation and not a pagination lib directly.

There was an open PR on Porpaginas recently preventing compatibility with PHP8: beberlei/porpaginas#23. That was recently merged and tests are passing on 2.0 now. That's been merged into master.

As for generics support. I do think we should support that.

@oprypkhantc what's an example of a non-list pagination type?

@oprypkhantc
Copy link
Contributor

@oojacoboo Well any pagination type isn't a list, technically. It's an object with one of the fields being list.

Other generic types, in the context of GraphQL, may include any kind of wrappers. Speaking of pagination, for GraphQL's cursor pagination spec we'll likely need generic "Connection" and a generic "Edge", both of which aren't iterables/lists as well.

@oojacoboo
Copy link
Collaborator

Yea, if we can add a check for iterable, that makes the most sense. Generics support was added recently and was a quick addition with a focus on iterables to support collection wrappers.

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

No branches or pull requests

3 participants