Airtable pagination support

[chriszarate]

Add support for pagination to Airtable queries. Airtable has a very odd pagination scheme, which it calls "offset" but is in fact a kind cursor pagination with a single cursor (instead of next and previous cursors).

Accordingly, I've added a new pagination type called "simple cursor" pagination. This required some refactoring of usePaginationVariables, but I took the opportunity to add tests. Thanks @brookewp for leading the way!

[github-actions]

Test this PR in WordPress Playground.

[brookewp]

Thanks for adding tests @chriszarate!

It makes sense to use renderHook as I assume this hook will be used in the upcoming Pagination block and not just in the ItemList component. However, in the docs for renderHook, there is this recommendation:

You should prefer render since a custom test component results in more readable and robust tests since the thing you want to test is not hidden behind an abstraction.

So what if we created a simplified custom component to capture the important behaviour from the user's perspective?

I think this would make it more flexible in the future, so we don't rely on implementation details, e.g. expect( result.current.supportsPagination ).toBe( false ).

I think it also may help to catch unexpected results. For example, in the first test we have expect( result.current.page ).toBe( 1 ), but there wouldn't be anything in the DOM for Page 1 if pagination isn't supported. So that could be a missed behaviour if we test this way.

Here's a rough example of what I'm thinking, first the test component and then using the same assertions from the supports page-based pagination test:

const TestComponent = ( { inputVariables }: { inputVariables: InputVariable[] } ) => {
	const pagination = usePaginationVariables( { inputVariables } );

	const hasFetched = useRef( false );
	useEffect( () => {
		if ( ! hasFetched.current ) {
			pagination.onFetch(
				generateRemoteData( 5, {
					totalItems: 100,
				} )
			);
			hasFetched.current = true;
		}
	}, [ pagination ] );

	return (
		<div>
			{ pagination.page && (
				<div role="option">
					Page { pagination.page } of { pagination.totalPages }
				</div>
			) }
			<div>{ pagination.perPage ?? pagination.totalItems } Items</div>
			<button onClick={ () => pagination.setPage( pagination.page + 1 ) }>
				Next Page
			</button>
			{ pagination.perPage && (
				<>
					<div>Items Per Page</div>
					<button onClick={ () => pagination.setPerPage( 5 ) } role="radio">
						5
					</button>
					<button onClick={ () => pagination.setPerPage( 25 ) } role="radio">
						25
					</button>
				</>
			) }
		</div>
	);
};

it( 'supports page-based pagination', async () => {
		const user = userEvent.setup();
		const inputVariables = [
			{
				required: false,
				slug: 'page',
				type: PAGINATION_PAGE_VARIABLE_TYPE,
			},
			{
				required: false,
				slug: 'perPage',
				type: PAGINATION_PER_PAGE_VARIABLE_TYPE,
			},
		];

		render( <TestComponent inputVariables={ inputVariables } /> );

		expect(
			screen.getByRole( 'option', {
				name: 'Page 1 of 20',
			} )
		).toBeVisible();

		expect( screen.getByText( '5 Items' ) ).toBeVisible();

		const nextPageButton = screen.getByRole( 'button', { name: 'Next Page' } );

		await user.click( nextPageButton );

		expect(
			screen.getByRole( 'option', {
				name: 'Page 2 of 20',
			} )
		).toBeVisible();

		await user.click( nextPageButton );

		expect(
			screen.getByRole( 'option', {
				name: 'Page 3 of 20',
			} )
		).toBeVisible();

		await user.click( screen.getByRole( 'radio', { name: '25' } ) );

		expect(
			screen.getByRole( 'option', {
				name: 'Page 3 of 4',
			} )
		).toBeVisible();

		expect( screen.getByText( '25 Items' ) ).toBeVisible();
	} );

What do you think?

[brookewp]

This looks good and is working well! I made a suggestion for the tests, but it's not a blocker in merging this.

[chriszarate]

I assume this hook will be used in the upcoming Pagination block and not just in the ItemList component.

This hook is not used directly by any component, it's composed by useRemoteData. Mostly this is to separate concerns and reduce the complexity of that (already complex) hook.

So I think your suggestion is absolutely what we should do for most hooks that are directly consumed by components. But for this hook, we are more concerned with the output values since they are directly consumed by another hook. It feels more like the "library" example in the linked doc. Does that sound right?

That said, we should probably also have higher-level tests for useRemoteData that take your suggested approach. So I'll work on those separately.

[chriszarate]

@brookewp and I had a nice chat and she fully convinced me that her suggested approach is universally better, and brings tons of benefits. My resistance is mostly focused on the complexity of the usePaginationVariables() hook and the perceived need for "unit-test-y" tests that are close to the implementation details.

But that's actually a signal that the code itself should be improved. It doesn't invalidate the fact that it is ultimately consumed by components that enable user behavior, and should be tested accordingly.

If there is complex functional logic, I can certainly pull that out in functions that are separately unit-tested. But the stateful composition of that logic is best tested with the approach Brooke suggested. I'm going to merge this and work on that separately. Thanks Brooke!