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

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

Closed
Tamxaun opened this issue Aug 1, 2022 · 14 comments · Fixed by #553
Labels
Bug Something isn't working like it should

Comments

@Tamxaun
Copy link

Tamxaun commented Aug 1, 2022

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 Tamxaun changed the title Pagination Query Store Pagination variable offset is null if using variables in query Aug 1, 2022
@Tamxaun Tamxaun changed the title Pagination variable offset is null if using variables in query If variable limit is used in query for pagination, variable offset in the request will be null Aug 1, 2022
@AlecAivazis
Copy link
Collaborator

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.

@Tamxaun
Copy link
Author

Tamxaun commented Aug 2, 2022

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 Tamxaun closed this as completed Aug 2, 2022
@Tamxaun Tamxaun reopened this Aug 2, 2022
@AlecAivazis AlecAivazis added the Bug Something isn't working like it should label Aug 4, 2022
@AlecAivazis
Copy link
Collaborator

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
Copy link
Collaborator

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

@Tamxaun
Copy link
Author

Tamxaun commented Sep 11, 2022

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 }
}

@AlecAivazis
Copy link
Collaborator

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
Copy link
Collaborator

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

@Tamxaun
Copy link
Author

Tamxaun commented Sep 12, 2022

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?

@jycouet
Copy link
Contributor

jycouet commented Sep 12, 2022

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

@Tamxaun
Copy link
Author

Tamxaun commented Sep 13, 2022

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?

@AlecAivazis
Copy link
Collaborator

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

@Tamxaun
Copy link
Author

Tamxaun commented Sep 15, 2022

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
   }
}

@AlecAivazis
Copy link
Collaborator

AlecAivazis commented Sep 19, 2022

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
Copy link
Collaborator

The original bug should now be fixed in 0.16.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working like it should
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants