relay-tools / relay-hooks

Use Relay as React hooks
https://relay-tools.github.io/relay-hooks/docs/relay-hooks.html
MIT License
540 stars 56 forks source link

usePagination bug: loadNext doesn't work when using a custom identifier field #215

Closed Lalitha-Iyer closed 5 months ago

Lalitha-Iyer commented 2 years ago

Description:

We use a custom id field _id and have configured the relay compiler to use this custom field as id. useQuery hook works fine however when using the usePagination hook and loadNext to fetch more items. I see the below error in the graphql response. message: "Variable '_id' has coerced Null value for NonNull type 'ID!'",…}

Fix
I could fix this locally by changing the id field in variables to be _id. We probably want to use identifierField instead of a fixed key. Something along these lines

            paginationVariables.id = identifierValue;     // Before
            paginationVariables[identifierField] = identifierValue;    // After

https://github.com/relay-tools/relay-hooks/blob/master/src/FragmentResolver.ts#L644

morrys commented 2 years ago

hi @Lalitha-Iyer, the logic of pagination is the same as how it is managed in relay. https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/useLoadMoreFunction.js#L201-L215

Can you edit the sample project (https://github.com/relay-tools/relay-hooks/tree/master/examples/relay-hook-example/pagination-nextjs-ssr) so that I can study the your case?

Lalitha-Iyer commented 2 years ago

sure thing, I will edit the project to have an example with a custom node id and try to repro the error. The customizable node id is a more recent development in relay. So it's possible the pagination in the official relay implementation also doesn't work as expected.

Lalitha-Iyer commented 2 years ago

I tried setting up the example, but am struggling to configure relay compiler with nextjs. I will give it a try again this week, otherwise would it be ok if I shared a code-sandbox example.

Lalitha-Iyer commented 1 year ago

Hi, @morrys I didn't get time to repro this in a sandbox. However there is now a relay issue that describes this with a repro

Lalitha-Iyer commented 1 year ago

Hi, @morrys I was looking at the solution proposed in the relay PR, looks similar to what I suggested here. See - packages/react-relay/relay-hooks/useLoadMoreFunction.js

Besides pagination variable we would also have to fix refetchable variable here https://github.com/relay-tools/relay-hooks/blob/2cda9c68ee5e69d996730f576a91603892cf771a/src/FragmentResolver.ts#L494

I am assuming the comment about @fetchable directive doesn't apply to us, hence we could just apply the simpler fix. https://github.com/facebook/relay/pull/4053#issuecomment-1231005869

What are your thoughts on fixing this issue? I am also happy to submit a PR with the fix if it helps.

morrys commented 1 year ago

Hi @Lalitha-Iyer, thanks for the tip 👍 For now, I'd like to wait to see how the PR in Relay goes. Let's keep in touch

Lalitha-Iyer commented 1 year ago

@morrys one workaround that seems to work is passing extra variables like so. Do you see any issues with doing this.

 loadNext(3, { UNSTABLE_extraVariables: { customId } }) 
Lalitha-Iyer commented 1 year ago

@morrys The PR in relay has been merged, whenever you get a chance could you revisit this issue. https://github.com/facebook/relay/pull/4053#issuecomment-1572598196

morrys commented 1 year ago

@Lalitha-Iyer As soon as relay releases PR with v16 version, I will modify relay-hooks in order to release v9 :)

morrys commented 5 months ago

released with relay-hooks v9.0.0