houdini icon indicating copy to clipboard operation
houdini copied to clipboard

If variable limit is used in query for pagination, variable offset in the request will be null

Open Tamxaun opened this issue 2 years ago • 3 comments

The following happens when I call loadNextPage on the Query Store and my query has variable limit and is not written by default (otherwise it's working correctly):

  1. First request is the same as initial
  2. Second and so on is with variable offset of null
query: "query TestPagination($offset: Int, $limit: Int) {\n  productsPaginateMod(offset: $offset, limit: $limit) {\n    name\n    id\n  }\n}\n"
variables: {limit: 1}
query: "query TestPagination($offset: Int, $limit: Int) {\n  productsPaginateMod(offset: $offset, limit: $limit) {\n    name\n    id\n  }\n}\n"
variables: {limit: 1, offset: null}

Query file:

query TestPagination($offset: Int, $limit: Int) {
	productsPaginateMod(
		offset: $offset
		limit: $limit
	) @paginate {
		name
	}
}

Svelte file:

<script lang="ts">
	import { browser } from '$app/env';
	import { GQL_TestPagination, getHoudiniContext } from '$houdini';

	const context = getHoudiniContext();
	const { loadNextPage } = GQL_TestPagination;

	$: browser && GQL_TestPagination.fetch({ variables: { limit: 1 } });
</script>

<div class="container grid py-8 mx-auto mb-20">
	<div>
		{#if $GQL_TestPagination.data != null}
			{#each $GQL_TestPagination.data.productsPaginateMod as product}
				<div>{product.name}</div>
			{/each}
		{:else}
			loading...
		{/if}
	</div>
	<div>
		<button
			class="p-2 mt-2 bg-gray-light"
			on:click={() => {
				loadNextPage(context);
				console.log($GQL_TestPagination);
			}}>load Next Page</button
		>
	</div>
</div>

Tamxaun avatar Aug 01 '22 11:08 Tamxaun

Hey @Tamxaun! Sorry to hear you're running into this.

Would you be able to put together a stackblitz that reproduces this issue? We have an integration test that verifies this behavior so it'd be really helpful to have something to compare to so we can see why you're situation is failing.

AlecAivazis avatar Aug 02 '22 05:08 AlecAivazis

I have created a repository on stackblitz. I used the public API to get the results. This example shows a problem with differences.

  1. The first request is correct
  2. The second request and so on with the error.
pagination.js:326

Uncaught (in promise) TypeError: currentOffset is not a function
    at Object.loadPage [as loadNextPage] (pagination.js:326:28)
    at HTMLButtonElement.click_handler (index.svelte? [sm]:34:31)

Tamxaun avatar Aug 02 '22 20:08 Tamxaun

Thanks for putting that together! I'm struggling to find the time to dig into this while making sure we're ready for the new svelte kit API. If you have the energy to look into it, I'd love to help however I can. Feel free to reach out on discord if you want a more direct method of communication.

AlecAivazis avatar Aug 04 '22 07:08 AlecAivazis

@Tamxaun do you mind checking if this is still a problem in houdini@next? I suspect it was fixed while I was reworking things

AlecAivazis avatar Sep 05 '22 00:09 AlecAivazis

The new version does not appear to have any errors. However, using @paginate in the query file and using variables produces some strange results.

Demo on stackblitz Demo on codesandbox

This case scenario displays one item initially and then displays all items later. As an example, if I added default value $limit: Int = 2 to it, later on it would not contain all items, but only 2 items.

# +page.gql
query SpacexHistories($limit: Int) {
   histories(limit: $limit, sort: "event_date_utc") @paginate {
      title
      details
      event_date_utc
   }
}
// +page.ts
import type { PageLoad } from './$types';
import { SpacexHistoriesStore } from "$houdini";

export const load: PageLoad = async (event) => {
   const SpacexHistories = new SpacexHistoriesStore();

   const result = await SpacexHistories.fetch({ event, variables: { limit: 1 } });

   console.log('do something with', result);

   return { SpacexHistories }
}

Tamxaun avatar Sep 11 '22 12:09 Tamxaun

Hey @Tamxaun! Is there a reason you are defining your load manually when you use a +page.gql file? Most of the time when you are using an approach with automatic loading, you shouldn't define your own load function since that will stop houdini from generating one for you.

AlecAivazis avatar Sep 11 '22 20:09 AlecAivazis

I forked the code sandbox example, removed the +page.js file and the data started loading as expected. I haven't dug too deep into what was the issue in the +page.js file but it's not reccomend to mix both approaches. Anyway, once that was done, i noticed that when I clicked on the load more i was getting all of the data (like you were seeing). I checked the network tab and the inputs are getting sent correctly but the result doesn't seem to be responding with the paginated fields (which is why the duplicates are showing up).

AlecAivazis avatar Sep 12 '22 07:09 AlecAivazis

I thought I had to create a file +page.js with a load function to pass the required variables.

Houdini makes its own load function, but how do we pass variables to the query?

Tamxaun avatar Sep 12 '22 10:09 Tamxaun

The runtime should tell you what is missing. Try deleting +page.js and it will say something like:

❌ Encountered error in src/routes/+page.js
Could not find required variable function: SpacexHistoriesVariables. maybe its not exported?

Then you can add again +page.ts and have something like:

import type { SpacexHistoriesVariables as Variables } from './$houdini';

export const SpacexHistoriesVariables: Variables = async ({ params }) => {
  // logic to get the limit... maybe from param? Maybe something else?
  const limit = params.limit || '1'
  
  return { limit };
};

The idea is to focus on what matters to you: variables. You can check: https://www.houdinigraphql.com/api/query as well and give us feedback.

Let us know

jycouet avatar Sep 12 '22 10:09 jycouet

After trying, I found that if variables are not required in the query file, the result remains the same as described above 🤔 Also, loadNextPage() no longer loads the next item (just current item if limit is 1), there is no offset.

Try deleting +page.js and it will say something like:

❌ Encountered error in src/routes/+page.js
Could not find required variable function: SpacexHistoriesVariables. maybe its not exported?

In addition, sometimes an error occurs about src/routes/+layout.js. Despite the existence of a +page.ts file with the function SpacexHistoriesVariables.

❌ Encountered error in src/routes/+layout.js
Could not find required variable function: SpacexHistoriesVariables. maybe its not exported?

Tamxaun avatar Sep 13 '22 12:09 Tamxaun

Do you mind sharing your query? Also, #542 will fix a lot of strange behavior with the layout files.

AlecAivazis avatar Sep 14 '22 20:09 AlecAivazis

This is my query, and demo on codesandbox.io shows the issue as well.

query SpacexHistories($limit: Int!) {
   histories(limit: $limit, sort: "event_date_utc") @paginate {
      title
      details
      event_date_utc
   }
}

Tamxaun avatar Sep 15 '22 20:09 Tamxaun

Okay, i was able to reproduce this 🎉 I'll try to figure out what's going on. Just to confirm - if you define your query like this, it works?

query SpacexHistories {
   histories(limit: 2, sort: "event_date_utc") @paginate {
      title
      details
      event_date_utc
   }
}

edit for a little more context: if you define the $limit variable explicitly like you are, then you will have to define the variable function to provide the value. Any time you have a query with inputs, you need a variable function

AlecAivazis avatar Sep 19 '22 18:09 AlecAivazis

The original bug should now be fixed in 0.16.5

AlecAivazis avatar Sep 21 '22 08:09 AlecAivazis