Inflect DataID field from Node interface. by alloy · Pull Request #2249 · facebook/relay

alloy · 2017-12-26T22:22:36Z

This PR makes it possible to inflect the DataID field to use from the user’s schema, rather than assuming it to be id.

How

The ID is either the single ID field of the Node interface in the user’s schema or otherwise falls back to id.
The ID field is selected using the __id alias.
At runtime the __id field in the response data is used as store DataID.

Example

If your schema looks like this:

interface Node {
  globalObjectID: ID!
}

type Artist implements Node {
  globalObjectID: ID!
  name: String!
}

…and you have a query like:

{
  artist(slug: "banksy") {
    name
  }
}

…then Relay will transform that to:

{
  artist(slug: "banksy") {
    __id: globalObjectID
    name
  }
}

…and use the value of __id as the key when writing the record to the store.

alloy · 2017-12-26T22:35:46Z


      const nextID =
-        item.id ||
+        (field.idField && item[field.idField]) ||


These two changes are the only runtime changes.

alloy · 2017-12-27T09:48:02Z

@josephsavona I got a couple of notification emails from GH with comments from you on this PR, but I don’t see them here. Is it a GH fluke or did you remove the comments?

josephsavona · 2017-12-27T11:00:23Z

Sorry about that. I meant to start a code review and accidentally posted not fully thought out comments instead - will take another look when I have computer time (holidays, baby, etc).

alloy · 2017-12-27T12:58:54Z

Oh no worries at all, I just got confused. Please enjoy your holidays and baby instead! 🙏 This can wait.

alloy · 2018-01-30T11:15:18Z

Rebased on master.

alloy · 2018-01-30T11:16:09Z

+      node.idField = idFieldSelection && idFieldSelection.name;
+    }
+    return node;
+  },


@josephsavona I think in your retracted initial review you said that this should become its own transformer, right? I think that makes sense, it’s kinda awkward here.

Yeah this should be its own transform rather than happening at codegen time

Will do 👍

josephsavona

I will leave a real review to @kassens and others since I'm no longer working on Relay. Other than the comment about splitting out transform-like code from codegen into a transform, the other thing that occurs to me here is the perf impact: adding an extra property (that is almost always the same redundant "id" value) to tons of fields increases the size of query modules, and it also incurs an extra condition/branch at runtime when reading.

josephsavona · 2018-01-30T17:14:11Z

+      node.idField = idFieldSelection && idFieldSelection.name;
+    }
+    return node;
+  },


Yeah this should be its own transform rather than happening at codegen time

alloy · 2018-01-30T20:23:41Z

adding an extra property (that is almost always the same redundant "id" value) to tons of fields increases the size of query modules

Ah yes, good point. I hadn’t considered bundle size enough, probably because 99% of our Relay code thus far exists in our mobile app.

If that turns out to be a major concern, then we can just special-case id and revert to the previous behaviour of not emitting idField and hardcoding checks for id at runtime.

it also incurs an extra condition/branch at runtime when reading

Related to the previous item, I think that even when reverting to special-casing the id field, this extra branch would remain because we’d have to first determine if an idField is specified before falling back to the hardcoded id field, otherwise we end up selecting an id field that should not ben selected.

leebyron

This looks cool! Some suggestions to continue:

This should be an extension of the existing transform which adds id fields
Rather than having to track idField on the resulting IR, you should use aliases, so that relay always knows to look for a specific response field that represents IDs.
I'd suggest looking for ID type in addition to ID!.

Also, a question. If a type has more than one fields with type ID!, what should be the right behavior?

alloy · 2018-02-11T13:47:33Z

Rather than having to track idField on the resulting IR, you should use aliases, so that relay always knows to look for a specific response field that represents IDs.

I like the simplicity of that idea 👌 What name would you suggest, is this where we use a name like __id?

I'd suggest looking for ID type in addition to ID!.

The Global ID spec mentions that this field is supposed to be non-null, but that’s probably more because of how Relay Classic was supposed to be able to refetch using that ID, rather it being needed for cache denormalisation (indeed Relay Modern will generate a client ID if the ID is missing from the response)? If so, sounds good to me 👍

Also, a question. If a type has more than one fields with type ID!, what should be the right behavior?

A type may, but the Node interface shouldn’t, which is what this PR is about. If that were to occur then this code will raise an assertion error.

The logic in this PR is:

If a Node interface exists in the schema, assume it has a single ID! field and use that
If no Node interface exists or the type does not implement the Node interface, check if the type has an id field and use that
Otherwise generate client IDs

alloy · 2018-02-14T23:20:36Z

@leebyron Ok, I think I addressed all your feedback, I’ve updated the original explanation to reflect this. The overall implementation is definitely a lot simpler 👌

There’s a fork of the TODO example app that uses this here https://github.com/alloy/relay-examples/tree/custom-node-id/todo and we’re pulling this into our app for further testing as well artsy/emission#985.

alloy · 2018-02-19T12:54:54Z

Rebased on master.

alloy · 2018-03-01T10:18:25Z

@leebyron FYI We’re now running this updated code in production in both our web and iOS app. You can see an example query here, but note that our schema uses __id for the GUID and thus most selections are just __id, only on line 150 will you find a __id: id selection.

alloy · 2018-03-23T10:30:05Z

@leebyron Could I get another review?

alloy · 2018-04-09T03:43:21Z

I’ve gone ahead and squashed all the commits now to ease rebasing on my end. I’ve also rebased, but alas can’t run the tests at the moment due to the issue described in #2407

alloy · 2018-04-09T23:03:06Z

Tests pass again 👍

jstejada

@alloy going to import this to test internally

Regarding this earlier comment:

If that turns out to be a major concern, then we can just special-case id and revert to the previous behaviour of not emitting idField and hardcoding checks for id at runtime.

Just want to double check, did you end up doing this? It looks like you did, but still always alias to __id?

jstejada · 2018-04-15T18:07:07Z

 ## Comparison to Classic Relay

-For users of classic Relay, note that the runtime makes as few assumptions as possible about GraphQL. Compared to earlier versions of Relay there is no concept of routes, there are no limitations on mutation input arguments or side-effects, arbitrary root fields just work, etc. At present, the main restriction from classic Relay that remains is the use of the `Node` interface and `id` field for object identification. However there is no fundamental reason that this restriction can't be relaxed (there is a single place in the codebase where object identity is determined), and we welcome feedback from the community about ways to support customizable object identity without negatively impacting performance.
+For users of classic Relay, note that the runtime makes as few assumptions as possible about GraphQL. Compared to earlier versions of Relay there is no concept of routes, there are no limitations on mutation input arguments or side-effects, arbitrary root fields just work, etc. Like classic Relay, modern Relay still uses the `Node` interface for object identification purposes; however, rather than always assuming the globally unique field to be `id`, modern Relay will inflect it from the `Node` interface by searching for either an `ID` or `ID!` field.


nit: can we use a different term than inflect, or not use it at all? (same for the title of this PR) I might just be me as a non-native english speaker, but I didn't know what inflect means, and googling the definition didn't really help until I actually looked at what this PR was doing.

maybe something like: "Relay Modern will find a field of type ID or ID! defined in the Node interface and use it as a globally unique ID"

jstejada · 2018-04-15T18:17:03Z

  RelayRelayDirectiveTransform.transform,
  RelayMaskTransform.transform,
  RelayDeferrableFragmentTransform.transform,
+  RelayGenerateIDFieldTransform.transform,


sorry I'm not super familiar with the compiler code. If we weren't using this transform here before, where were we using it?

jstejada · 2018-04-15T18:28:26Z

- * A transform that adds an `id` field on any type that has an id field but
- * where there is no unaliased `id` selection.
+ * A transform that adds a `__id` field on any type that has a `Node` or `id`
+ * field but where there is no unaliased `__id` selection.


so we'll always add an __id selection, unless there's already an unaliased __id selection.

what happens when there's already an aliased __id selection?, will we add another __id selection too?

jstejada · 2018-04-15T18:29:38Z

-  // If the field already has an unaliased `id` field, do nothing
-  if (hasUnaliasedSelection(field, ID)) {
+  // If the field already has an `__id` selection, do nothing.
+  if (hasSelection(node, '__id')) {


answering my own question from above, it seems that this won't add an __id selection if a selection for __id already exists, be it aliased or unaliased. Maybe update the comment above to reflect that?

jstejada · 2018-04-15T18:31:38Z

-  return IRTransformer.transform(
-    context,
-    {
-      LinkedField: visitLinkedField,


sorry I'm probably missing some context, why did we previously only apply this transform to LinkedFields, and now we do so for Fragments too?

jstejada · 2018-04-15T18:32:34Z

-  // implement `Node`
+  // - If the field type is abstract, then generate a `... on Node { __id: id }`
+  //   fragment if *any* concrete type implements `Node`. Then generate a
+  //   `... on PossibleType { __id: id }` for every concrete type that does


what if PossibleType doesn't have an id field? can we add test coverage for this?

facebook-github-bot

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

alloy · 2018-08-04T16:31:33Z

New TODOs

RelayDeferrableFragmentTransform hardcodes id
RelayViewerHandleTransform hardcodes id
RelayValidator disallows id alias
RelayQLPrinter may still be generating id fields

* The ID is either the single `ID` field of the `Node` interface in the user’s schema or otherwise falls back to `id`. * The ID field is selected using the `__id` alias.

alloy · 2018-08-04T17:34:07Z

This rebased version has a couple of failing tests, that I have left in as a good reminder of what to work on and I also need to address @jstejada’s feedback, but for now I want to first see if it actually works in our apps.

facebook-github-bot

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

clentfort · 2019-06-03T08:23:08Z

What is the current progress on this issue? Is there any way one could help to advance this PR?

alloy · 2019-06-03T11:23:21Z

This patch has served us well, but we’ve decided that we are going to sunset this PR. The reasons are:

Redoing this PR for current master is non-trivial and was a constant source for us not being able to upgrade to latest Relay easily.
Since the creation of this patch, the GraphQL spec has decided to reserve fields starting with a dunderscore for internal use only, so even when/if we’d ever land this we at Artsy would still be left with a schema that all of our tooling has started complaining about.

In the end, we decided to start experimentation with a v2 of our schema in which we transform v1 to rename all of our ID fields to conform to the Node spec. Things are looking good thus far and we’ve been able to upgrade to Relay 4 easily.

alloy · 2019-06-03T11:28:47Z

@clentfort What a coincidence that you would ask that on the day that I was planning on sunsetting this PR 😞

If you are interested in taking a solution like this to the finish line, some work has been done to being able to provide a custom DataID value at runtime. However, unless your DataID would rely on id and __typename, which are already being queried for by default by relay, you would have to do some work to provide your own query transform that, at compile time, inserts the fields you need selected.

facebook-github-bot added the CLA Signed label Dec 26, 2017

alloy force-pushed the custom-node-id-redone branch from 55f65a2 to 705e7a2 Compare December 26, 2017 22:26

alloy commented Dec 26, 2017

View reviewed changes

sibelius mentioned this pull request Jan 17, 2018

relay-compiler Expected undefined to be a GraphQL leaf type. #2285

Closed

alloy mentioned this pull request Jan 29, 2018

Throw better error for missing idType #2290

Closed

alloy force-pushed the custom-node-id-redone branch from 2cc1e49 to 2ca1486 Compare January 30, 2018 11:14

alloy commented Jan 30, 2018

View reviewed changes

josephsavona reviewed Jan 30, 2018

View reviewed changes

alloy force-pushed the custom-node-id-redone branch from 2ca1486 to 3e3decb Compare February 3, 2018 22:16

alloy mentioned this pull request Feb 6, 2018

[Relay] Uses latest version and our patches artsy/emission#982

Merged

leebyron suggested changes Feb 8, 2018

View reviewed changes

alloy force-pushed the custom-node-id-redone branch from 3e3decb to 6fb1d8d Compare February 14, 2018 20:33

alloy changed the title ~~Inflect DataID field from Node interface.~~ [WIP] Inflect DataID field from Node interface. Feb 14, 2018

alloy mentioned this pull request Feb 14, 2018

[Relay] Update to latest versions of Relay and our patches. artsy/emission#985

Merged

alloy added a commit to alloy/relay-examples that referenced this pull request Feb 14, 2018

[package] Update for latest version of facebook/relay#2249

fadc903

damassi mentioned this pull request Feb 14, 2018

[Toolchain] Update Relay to support new TypeScript compiler option artsy/reaction#522

Merged

alloy changed the title ~~[WIP] Inflect DataID field from Node interface.~~ Inflect DataID field from Node interface. Feb 14, 2018

alloy force-pushed the custom-node-id-redone branch 2 times, most recently from 4098931 to ee71c36 Compare February 19, 2018 12:54

alloy force-pushed the custom-node-id-redone branch from ee71c36 to b5e4b8f Compare April 9, 2018 03:42

alloy force-pushed the custom-node-id-redone branch from b5e4b8f to 3ca8f8c Compare April 9, 2018 22:57

jstejada reviewed Apr 15, 2018

View reviewed changes

facebook-github-bot reviewed Apr 15, 2018

View reviewed changes

[GenerateIDFieldTransform] Inflect ID from Node and select as __id.

20eeddb

* The ID is either the single `ID` field of the `Node` interface in the user’s schema or otherwise falls back to `id`. * The ID field is selected using the `__id` alias.

alloy force-pushed the custom-node-id-redone branch from 3ca8f8c to 20eeddb Compare August 4, 2018 17:33

facebook-github-bot reviewed Apr 25, 2019

View reviewed changes

alloy closed this Jun 3, 2019

Conversation

alloy commented Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How

Example

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alloy commented Dec 27, 2017

Uh oh!

josephsavona commented Dec 27, 2017

Uh oh!

alloy commented Dec 27, 2017

Uh oh!

alloy commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephsavona left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alloy commented Jan 30, 2018

Uh oh!

leebyron left a comment

Choose a reason for hiding this comment

Uh oh!

alloy commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alloy commented Feb 14, 2018

Uh oh!

alloy commented Feb 19, 2018

Uh oh!

alloy commented Mar 1, 2018

Uh oh!

alloy commented Mar 23, 2018

Uh oh!

alloy commented Apr 9, 2018

Uh oh!

alloy commented Apr 9, 2018

Uh oh!

jstejada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

alloy commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alloy commented Aug 4, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

clentfort commented Jun 3, 2019

Uh oh!

alloy commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alloy commented Jun 3, 2019

Uh oh!

alloy commented Dec 26, 2017 •

edited

Loading

alloy commented Feb 11, 2018 •

edited

Loading

alloy commented Aug 4, 2018 •

edited

Loading

alloy commented Jun 3, 2019 •

edited

Loading