support simple pagination

[lamtranb]

Summary

---

Hello, as #709 questioned, I think we should support simplePagination.
As Laravel doc mentioned here: https://laravel.com/docs/8.x/pagination#simple-pagination

However, if you do not plan to show the total number of pages in your application's UI then the record count query is unnecessary.

If we have a large table then this will be a problem.

Type of change:

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

Checklist:

• Existing tests have been adapted and new tests have been added
• Add a CHANGELOG.md entry
• Update the README.md
• Code style has been fixed via composer fix-style

[mfn]

Very nice @lamtranb !

Approach LGTM, can you check my feedback and also add a changelog entry?

PS: should phpstsan still complain, it's fine -> I can take care of that afterwards

Thanks!

[mfn]

I don't think this field can ever return null:

Suggested change

	'type' => GraphQLType::listOf(GraphQL::type($typeName)),
	'type' => Type::nonNull(GraphQLType::listOf(GraphQL::type($typeName))),

[lamtranb]

Type::nonNull seems not work with ListOf. ListOf can return an empty list. I will add a new test case for this. Or you mean this?
GraphQLType::listOf(Type::nonNull(GraphQL::type($typeName)))

[mfn]

Good question!

Type::nonNull seems not work with ListOf

Oh, but it should!

In this case I meant that the value returned by resolve cannot be null, ever, because even the closure has the typehint Collection.

This means there will always be a collection returned, never null, so wrapping the existing GraphQLType::listOf(GraphQL::type($typeName)), into non-null like GraphQLType::nonNull(GraphQLType::listOf(GraphQL::type($typeName))), should be possible!

(sorry I see in my feedback I just write Type::nonNull which is usually the alias used, but in this class it's GraphQLType, so maybe that's why it may look confusing).

ListOf can return an empty list

Exactly! But "empty list" !== "null"!

Or you mean this? GraphQLType::listOf(Type::nonNull(GraphQL::type($typeName)))

I would love to (always good to avoid null values, if possible), but I think a collection containing null values is technically allowed (however unlikely), so therefore sadly no, didn't meant this one.

[lamtranb]

Hi @mfn
Thanks for taking your time to review my code.

I updated the code into this:
GraphQLType::nonNull(GraphQLType::listOf(GraphQL::type($typeName)))
The tests failed.
I checked in the file tests/Database/SelectFields/PrimaryKeyTests/PrimaryKeySimplePaginationQuery.php
/** @var SelectFields $selectFields */ $selectFields = $getSelectFields(); $selectFields->getSelect()
only return posts.id. If I remove GraphQLType::nonNull, it will return posts.id, posts.title. I think there is a bug in src/Support/SelectFields.php or miss check somewhere. Can you help me? I'm not familiar with this. Thanks very much.

[mfn]

TBH I've no idea.

I don't consider it a showstopper and I also didn't write the existing PaginationType which works exactly the same way, so I actually do feel bad for side-tracking this 😞

[mfn]

From #715 (comment)

Anonymous function never returns null so it can be removed from the return typehint.

I guess the framework code itself is contradicting, see https://github.com/laravel/framework/blob/6319d94ccaf8169b103813a82139814573ddef09/src/Illuminate/Pagination/AbstractPaginator.php#L321-L325

    /**
     * Get the number of the first item in the slice.
     *
     * @return int
     */
    public function firstItem()
    {
        return count($this->items) > 0 ? ($this->currentPage - 1) * $this->perPage + 1 : null;
    }

The phpdoc (which phpstan obliges) mentions only int but in the code we clearly see it's possible to return null -> i.e. think about empty collections.

Same for to, in fact.

🤷‍♀️

Anyway, I "fixed" this one by adding it to phpstans baseline aka silencing it.

Can you look at the remaining feedback?

Thanks!

[mfn]

Good question!

Type::nonNull seems not work with ListOf

Oh, but it should!

In this case I meant that the value returned by resolve cannot be null, ever, because even the closure has the typehint Collection.

This means there will always be a collection returned, never null, so wrapping the existing GraphQLType::listOf(GraphQL::type($typeName)), into non-null like GraphQLType::nonNull(GraphQLType::listOf(GraphQL::type($typeName))), should be possible!

(sorry I see in my feedback I just write Type::nonNull which is usually the alias used, but in this class it's GraphQLType, so maybe that's why it may look confusing).

ListOf can return an empty list

Exactly! But "empty list" !== "null"!

Or you mean this? GraphQLType::listOf(Type::nonNull(GraphQL::type($typeName)))

I would love to (always good to avoid null values, if possible), but I think a collection containing null values is technically allowed (however unlikely), so therefore sadly no, didn't meant this one.

[mfn]

LGTM, thank you very much!

[mfn]

TBH I've no idea.

I don't consider it a showstopper and I also didn't write the existing PaginationType which works exactly the same way, so I actually do feel bad for side-tracking this 😞

[mfn]

Released as https://github.com/rebing/graphql-laravel/releases/tag/7.0.1 !