Fix refetching and pagination when using a custom nodeInterfaceIdField #4053

Emilios1995 · 2022-08-27T00:44:28Z

This fixes #3897.

I wasn't able to get the tests to pass on my machine, even on main, so I didn't consider whether to add tests for this change.
Please let me know whether I should do something other than yarn install, yarn build and yarn test for them to work.

Still, I verified that the changes fix my issue by testing on a project that uses the custom id field. I tested both: Refetching, and loading more data on a pagination fragment.

josephsavona

Overall this looks good, some small nits. Could you please create at least one unit test (see existing useRefetchableFragment unit tests) to ensure that this feature doesn't break? Thanks!

josephsavona · 2022-08-29T16:05:28Z

packages/react-relay/relay-hooks/useRefetchableFragmentNode.js

+      fragmentNode: ReaderFragment,
+      componentDisplayName: string,


nit: pass identifierField instead of recomputing it. i would also default to id if identifierField is null/undefined

josephsavona · 2022-08-29T16:07:06Z

packages/react-relay/relay-hooks/useRefetchableFragmentNode.js

+      const id =
+        identifierField !== null &&
+        identifierField !== undefined &&
+        identifierField !== ''
+          ? memoRefetchVariables?.[identifierField]
+          : null;


see above, then you could remove the null/undefined checks here. also note that we generally use x == null to check for both null or undefined (this is the one place where it's reasonable to use double equals).

Emilios1995 · 2022-08-30T00:28:44Z

Overall this looks good, some small nits. Could you please create at least one unit test (see existing useRefetchableFragment unit tests) to ensure that this feature doesn't break? Thanks!

Thanks for the review @josephsavona! Unfortunately, after running the existing tests, I realized that my changes break some existing functionality related to refetching or paginating @fetchable fragments.

Specifically, when paginating or refetching a @fetchable fragment, the query that the compiler generates accepts a variable named id, even though the fragment's identifierField might be something else, like fetch_id, for example.

This is unlike the compiler's behavior when setting the nodeInterfaceField setting: In that case, the query that the compiler generates accepts a variable with the same name as identifierField. This is the behavior I had in mind when making this PR, but it breaks the tests related to refetching @fetchable types.

I think we can go forward in three different directions:

Make the hooks always pass both variables to the query: identifierField and "id". The former covers use cases where nodeInterfaceField is set, and the later everything else, including @fetchable fragments. It would look something like this.
Is there some way we can get the nodeInterfaceField setting at runtime? If so, the hook can use that value to know what variable to pass, rather than identifierField as in the current state of this PR.
Maybe Allow Node interface id field to be configurable. #3638 was wrong in setting the query's variable name to be nodeInterfaceField / identifierField. Instead, it should have remained just id to be consistent with the behavior for @fetchable types.

IMO, option 1 is very pragmatic, but both 1 and 2 are a bit leaky since they force the hooks to be aware of the compiler's quirks. Option 3 sounds more correct to me. However, adopting it would be a breaking change since some users of nodeInterfaceField might already be going around this bug by manually passing the id variable to the refetch/pagination variables. (Though I'm not sure how much we should worry about a breaking change affecting something that was always a workaround anyway!)

What do you think?

josephsavona · 2022-08-30T17:41:32Z

is there some way we can get the nodeInterfaceField setting at runtime?

It should already be available on the refetch metadata (?). But approach #2 seems reasonable generally, the compiler can emit metadata in the AST to be consumed at runtime.

Emilios1995 · 2022-09-02T17:52:13Z

@josephsavona The scope of this PR increased a bit. Based on your comments, I decided to add another field to the refetchable fragments' metadata, since as per my previous comments, the identifierField metadata wasn't enough for the hooks to effectively determine what the query variable should be named.

The new metadata field is pretty explicit so the hooks don't need to have any complex logic to figure it out. It's named identifierQueryVariableName and it is exactly what the name conveys.

I know you asked for a test in the hooks to make sure this doesn't break, but unfortunately that's not straightforward. Currently all the GraphQL operations for the tests are being compiled by the relay-compiler as configured in scripts/config.tests.json. This configuration configures the packages folder as its source.

This makes it impossible to create a new test suite whose GraphQL ops are compiled with a different compiler setting, which is what we would need to test with the nodeInterfaceIdField setting.

There are ways around that but all of them would have a pretty heavy footprint on how the codebase is structured. (For example, one solution could be to exclude the generated artifacts from source control, and generate them on demand before the tests are run. This would allow us to compile and run the test suite as normal, compile it again with the nodeInterfaceIdField set, and then do a second run of jest but with CLI args so that it only runs a subset of the test, namely, the ones that are relevant to the nodeInterfaceIdField such as the refetchable and pagination ones.)

I think that with this change, adding a test to the hooks becomes less important since now the compiler is responsible for explicitly letting the hooks know what the variable should be named. The hooks should just use the provided metadata field, so there isn't any special logic it has to perform in the case when nodeInterfaceIdField is set.

If you insist on having a new test for the hooks, I would require instruction on how exactly to make that happen (given the need of a separate compiler config as I explained above.)

Emilios1995 · 2022-09-15T15:43:55Z

@josephsavona Have you had a chance to look at my latest changes?

kav · 2022-12-03T00:11:10Z

Any chance of getting this or something similar merged @josephsavona?

captbaritone

Overall this looks good, and thanks for working on it! I left a few comments. Most are trivial, but there is one about allowing the variable name to be configured independently of the field name. Curious to hear your thoughts.

Apologies for the slow response on this PR.

captbaritone · 2023-01-05T17:34:33Z

packages/react-relay/relay-hooks/useRefetchableFragmentNode.js

-        identifierField != null &&
-        !providedRefetchVariables.hasOwnProperty('id')
+        identifierQueryVariableName != null &&
+        !providedRefetchVariables.hasOwnProperty(identifierQueryVariableName)


See my comment on useRefetchableFragmentInternal_REACT_CACHE.js above. I think this should probably include an invariant?

captbaritone · 2023-01-05T17:49:18Z

packages/react-relay/relay-hooks/react-cache/useRefetchableFragmentInternal_REACT_CACHE.js

@@ -448,8 +455,8 @@ function useRefetchFunction<TQuery: OperationType>(
      // If the query needs an identifier value ('id' or similar) and one
      // was not explicitly provided, read it from the fragment data.
      if (
-        identifierField != null &&
-        !providedRefetchVariables.hasOwnProperty('id')
+        identifierQueryVariableName != null &&


I see. So previously we used the existence of identifierField to tell us if we are in an operation that needs an identifier. Now we use identifierQueryVariableName and the use the warning below to ensure we also have an identifierField?

If there's a compiler invariant that we always have an identifierField when we have an identifierQueryVariableName, maybe we should at least add an invariant call inside this if statement`. For bonus points we could restructure the compiler output to be an optional object that contains both values.

So, not quite. The warning below was already there before this PR. It was necessary because it doesn't only check for the existence of identifierField, rather, it makes sure we have an identifierValue, which is defined as fragmentData[identifierField].

Even if the compiler invariant holds and we have a identifierField whenever there is an identifierQueryVariableName, identifierValue might be missing for a number of different reasons outside the control of the compiler. For example, there might be a malfunction in the Relay runtime or perhaps even in the users's GraphQL server.

Nevertheless, if we hit the warning we're definitely in a corrupted state and there's a case to be made for triggering an exception via an invariant, but I would you to confirm that this is indeed what we want since I'm not familiar with the team's heuristics for when to use a warning and when to go for an invariant.

Changing the compiler output as you're describing would still be nice. I'll see if I can get around to doing it.

captbaritone · 2023-01-05T17:50:07Z

packages/react-relay/relay-hooks/useLoadMoreFunction.js

@@ -196,7 +200,7 @@ function useLoadMoreFunction<TQuery: OperationType>(

      // If the query needs an identifier value ('id' or similar) and one
      // was not explicitly provided, read it from the fragment data.
-      if (identifierField != null) {
+      if (identifierQueryVariableName != null) {


See my comment on useRefetchableFragmentInternal_REACT_CACHE.js above. I think this should probably include an invariant?

captbaritone · 2023-01-05T17:52:10Z

compiler/crates/relay-codegen/src/build_ast.rs

@@ -436,6 +436,13 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
                });
            }

+            if let Some(x) = refetch_metadata.identifier_query_variable_name {


Nit: Can we have a more descriptive variable name than x?

captbaritone · 2023-01-05T18:09:50Z

compiler/crates/relay-transforms/src/refetchable_fragment/node_query_generator.rs

@@ -120,6 +120,7 @@ fn build_refetch_operation(
                        operation_name: query_name,
                        path: vec![CONSTANTS.node_field_name],
                        identifier_field: Some(id_name),
+                        identifier_query_variable_name: Some(id_name),


Here we assume the variable name will always match the field name. I've seen cases where people would like to deviate from that assumption. What do you think about adding a schema config option parallel to node_interface_id_field called node_interface_id_variable_name or similar?

That would also be useful for the case where someone was depending upon the existing behavior.

Emilios1995 · 2023-01-30T16:40:35Z

Overall this looks good, and thanks for working on it! I left a few comments. Most are trivial, but there is one about allowing the variable name to be configured independently of the field name. Curious to hear your thoughts.

Apologies for the slow response on this PR.

Thanks for the review! I added a comment about the invariant you proposed.

Regarding configuring the variable name, I agree it would be a useful feature. I'll add it to the PR and let you know when it's ready for another review!

morrys · 2023-03-01T20:43:23Z

any news on this PR? 👍

Emilios1995 · 2023-03-03T03:31:01Z

any news on this PR? 👍

I've been a bit busy lately, but this week I started implementing the changes that Jordan requested. They'll be ready soon!

Emilios1995 · 2023-03-20T00:34:03Z

Hey @captbaritone! I finally got around to implementing the changes you requested.

Please take a look at the last three commits. The first two of them add support for configuring the variable name, as you suggested. (The default keeps the current beahvior.)
The last commit implements the "extra point" change you suggested. Namely, changing the compiler output so that the query variable name and the identifier field live as non-nullable fields inside a new struct type. This change is propagated to the interfaces exposed from relay-runtime, helping consumers such as the flow code in react-relay avoid having to handle "impossible states" where one of the fields is present and the other is not.

captbaritone · 2023-03-20T16:57:10Z

packages/relay-runtime/yarn.lock

@@ -0,0 +1,111 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


Looks like this got checked in by accident.

captbaritone · 2023-03-20T16:57:31Z

packages/react-relay/yarn.lock

@@ -0,0 +1,125 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


Looks like this got checked in by accident.

captbaritone · 2023-03-20T17:00:08Z

packages/relay-runtime/util/getRefetchMetadata.js

+      identifierInfo.identifierField == null ||
+        typeof identifierInfo.identifierField === 'string',
+      'Relay: getRefetchMetadata(): Expected `identifierField` to be a string.',
+    );


Should we add a similar invariant for identifierQueryVariableName?

Sure, it doesn't hurt and it's consistent with the rest of the function. Added!

packages/relay-runtime/util/getRefetchMetadata.js

captbaritone · 2023-03-20T17:20:57Z

packages/react-relay/relay-hooks/useLoadMoreFunction.js

@@ -201,7 +207,7 @@ function useLoadMoreFunction<TVariables: Variables>(

      // If the query needs an identifier value ('id' or similar) and one
      // was not explicitly provided, read it from the fragment data.
-      if (identifierField != null) {
+      if (identifierInfo != null) {


I'm confused by the relationship between identifierField and identifierInfo in this scope. My sense right now is that they are actually both coming from the same metadata but that identifierInfo is extracted locally and identifierField is extracted by the parent function and passed in. Is that correct?

If not, maybe we could add a comment explaining the difference between the two? If not, maybe we could get rid of one of them and either always pass in a full identifierInfo object, or avoid the need for parent functions to pass it in, and instead always extract it locally?

You're right, having both was redundant. I just changed it so that this function doesn't need parent functions to pass identifierInfo. Now we just extract it locally.

captbaritone · 2023-03-20T17:24:12Z

Thanks for your continued work on this @Emilios1995! Left another round of comments.

Co-authored-by: Jordan Eldredge <jordan@jordaneldredge.com>

Emilios1995 · 2023-03-28T04:26:09Z

Thanks for your continued work on this @Emilios1995! Left another round of comments.

Thank you @captbaritone! I just pushed some changes addressing your comments.

Emilios1995 · 2023-05-17T17:37:25Z

@captbaritone Hey! Any chance we could get this merged?

facebook-github-bot · 2023-05-17T18:56:53Z

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

captbaritone · 2023-05-22T23:18:52Z

Quick update here. I've managed to rebase and adapt this PR to work internally. It's now up for review internally.

facebook-github-bot · 2023-05-31T21:55:31Z

@captbaritone merged this pull request in 0fce632.

captbaritone · 2023-06-01T18:45:12Z

This has landed. Folks should be able to try it out using the @main tag of the NPM packages. Note that you'll need to upgrade both the relay-runtime and relay-compiler packages.

Emilios1995 · 2023-06-08T19:25:53Z

This has landed. Folks should be able to try it out using the @main tag of the NPM packages. Note that you'll need to upgrade both the relay-runtime and relay-compiler packages.

Thank you, @captbaritone!

Lalitha-Iyer · 2024-01-19T17:16:22Z

Thank @Emilios1995 for this! Works great.

Lalitha-Iyer · 2024-01-19T17:17:11Z

One thing I had to figure out was adding nodeInterfaceIdVariableName in the schema config since we used a different variable name than "id". I don't see any docs on custom node id, so probably worth documenting this along with other
missing parts.

kav · 2024-01-19T18:00:26Z

It's worth noting at present it appears the default implementation of getDataID sill uses a hardcoded value of id rather than looking up the nodeInterfaceIdField value so needs to be provided as well.

getDataID: (fieldValue) => fieldValue.nodeId

#4199 (comment)

Emilios1995 · 2024-01-21T23:24:37Z

@Lalitha-Iyer That's a good point. We did document the new variable here but I can see how it would be better to have a comprehensive guide documenting all the customization options.

Such a guide would also be a good place to remind people they might need to customize the getDataID too, as @kav highlighted.

@captbaritone Would you like there to be a new Guide on relay.dev called "Using a different field for global IDs" or something similar documenting everything?

facebook-github-bot added the CLA Signed label Aug 27, 2022

Emilios1995 changed the title ~~Fix refetching and pagination when using a custom #3897~~ Fix refetching and pagination when using a custom nodeInterfaceIdField Aug 27, 2022

josephsavona requested changes Aug 29, 2022

View reviewed changes

Emilios1995 marked this pull request as draft September 2, 2022 04:16

Emilios1995 force-pushed the fix-alt-data-id branch 2 times, most recently from 5702c6e to f6c69f0 Compare September 2, 2022 16:55

Emilios1995 marked this pull request as ready for review September 2, 2022 17:52

Emilios1995 requested a review from josephsavona September 2, 2022 17:52

kav mentioned this pull request Nov 23, 2022

add support for Relay.js supabase/pg_graphql#209

Closed

Lalitha-Iyer mentioned this pull request Dec 13, 2022

usePagination bug: loadNext doesn't work when using a custom identifier field relay-tools/relay-hooks#215

Closed

captbaritone reviewed Jan 5, 2023

View reviewed changes

Emilios1995 force-pushed the fix-alt-data-id branch from f6c69f0 to 49e0059 Compare March 5, 2023 20:54

Emilios1995 marked this pull request as draft March 6, 2023 01:56

Emilios1995 force-pushed the fix-alt-data-id branch from 6750d06 to 9f3f74d Compare March 6, 2023 02:06

Emilios1995 added 8 commits March 19, 2023 18:26

Fix refetching when using nodeInterfaceIdField

ed43b5f

Fix pagination when using nodeInterfaceIdField

45c5ee5

Add variable name to refetchable fragment metadata

284b375

use the new metadata field on hooks

d739d71

update generated tests files

667bf9a

Use the new metadata field on react-cache hooks

74f72f4

Support configuring the node query variable

c9bbb95

Add tests with custom node query variable name

c5be931

Emilios1995 force-pushed the fix-alt-data-id branch from 9f3f74d to c3866a7 Compare March 20, 2023 00:27

Emilios1995 marked this pull request as ready for review March 20, 2023 00:33

captbaritone requested changes Mar 20, 2023

View reviewed changes

Emilios1995 and others added 4 commits March 27, 2023 20:56

Update packages/relay-runtime/util/getRefetchMetadata.js

f81e8c2

Co-authored-by: Jordan Eldredge <jordan@jordaneldredge.com>

Remove identifierField arg from useLoadMoreFunction

a91dbb9

Add invariant to getRefetchMetadata

b2206b4

Remove files checked-in by mistake

2bf5849

Emilios1995 requested review from captbaritone and josephsavona and removed request for josephsavona and captbaritone March 29, 2023 18:36

captbaritone mentioned this pull request May 17, 2023

Use same field name for pagination that compiler generates #4317

Closed

facebook-github-bot closed this in 0fce632 May 31, 2023

facebook-github-bot added the Merged label May 31, 2023

		@@ -0,0 +1,111 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

		@@ -0,0 +1,125 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

Fix refetching and pagination when using a custom nodeInterfaceIdField #4053

Fix refetching and pagination when using a custom nodeInterfaceIdField #4053

Conversation

Emilios1995 commented Aug 27, 2022 • edited Loading

josephsavona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emilios1995 commented Aug 30, 2022 • edited Loading

josephsavona commented Aug 30, 2022 • edited Loading

Emilios1995 commented Sep 2, 2022

Emilios1995 commented Sep 15, 2022

kav commented Dec 3, 2022

captbaritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emilios1995 commented Jan 30, 2023

morrys commented Mar 1, 2023

Emilios1995 commented Mar 3, 2023

Emilios1995 commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captbaritone commented Mar 20, 2023

Emilios1995 commented Mar 28, 2023

Emilios1995 commented May 17, 2023

facebook-github-bot commented May 17, 2023

captbaritone commented May 22, 2023

facebook-github-bot commented May 31, 2023

captbaritone commented Jun 1, 2023

Emilios1995 commented Jun 8, 2023

Lalitha-Iyer commented Jan 19, 2024

Lalitha-Iyer commented Jan 19, 2024 • edited Loading

kav commented Jan 19, 2024

Emilios1995 commented Jan 21, 2024

Emilios1995 commented Aug 27, 2022 •

edited

Loading

Emilios1995 commented Aug 30, 2022 •

edited

Loading

josephsavona commented Aug 30, 2022 •

edited

Loading

Lalitha-Iyer commented Jan 19, 2024 •

edited

Loading