Relay: [help wanted] jest tests not passing on node 11

[kassens]

When enabling travis for node 11 or running node 11 locally, some compiler tests fail with keys of the generated code being in different order.

I assume there's some edge case bug in Map in the V8 version used by node 11. Would be good to debug and potentially file an upstream bug with a reduced use case.

If someone wants to investigate this, please say so in the comments here so we don't duplicate the effort!

[alloy]

We've been noticing that artefacts get written to disk in a different order on Node 11 and determined that JSON.stringify, used here, serialises in a different order. Not sure if that was due to the type of data structure passed to the function, though.

I couldn’t easily find any Node release notes about this, but also didn’t search much more, as for our case we simply determined people should be using the same Node version as the rest of us, which is v10.

[artola]

@alloy @kassens Might be related to v11 change: https://github.com/nodejs/node/issues/24294 ?

[kassens]

JSON.stringify should use the iteration order. Maybe we have a not-fully-specified order so the change that @artola pointed out might be the reason. The fix would be to find that custom comparator and make the ordering absolute instead of partial we might be using somewhere.

[kassens]

@artola that's exactly it!
Commenting out this sort gets us the same result across node versions:
https://github.com/facebook/relay/blob/40c8c915f768d9fa5b0d27449fb52498fc0e5907/packages/relay-compiler/transforms/SkipRedundantNodesTransform.js#L210

The fix would be to make it an absolute ordering so all versions produce the same predictable output.

[eliperkins]

I can pick this one up. Can you clarify a bit more what you mean by "absolute ordering" @kassens?

[eliperkins]

After toying around with this for a bit, it seems like any change to sortSelections is going to be a breaking change, as our current ordering is not stable.

Currently, failures on Node 11 look something like this:

@@ -56,51 +56,51 @@
        "concreteType": null,
        "plural": false,
        "selections": [
          {
            "kind": "InlineFragment",
-           "type": "MarkdownUserNameRenderer",
+           "type": "PlainUserNameRenderer",
            "selections": [
              {
                "kind": "ModuleImport",
                "fragmentPropName": "name",
-               "fragmentName": "MarkdownUserNameRenderer_name"
+               "fragmentName": "PlainUserNameRenderer_name"
              },
              {
                "kind": "ScalarField",
                "alias": "__module_component",
                "name": "js",
                "args": [
                  {
                    "kind": "Literal",
                    "name": "module",
-                   "value": "MarkdownUserNameRenderer.react",
+                   "value": "PlainUserNameRenderer.react",
                    "type": "String"
                  }
                ],
                "storageKey": "__module_component"
              }
            ]
          },
          {
            "kind": "InlineFragment",
-           "type": "PlainUserNameRenderer",
+           "type": "MarkdownUserNameRenderer",
            "selections": [
              {
                "kind": "ModuleImport",
                "fragmentPropName": "name",
-               "fragmentName": "PlainUserNameRenderer_name"
+               "fragmentName": "MarkdownUserNameRenderer_name"
              },
              {
                "kind": "ScalarField",
                "alias": "__module_component",
                "name": "js",
                "args": [
                  {
                    "kind": "Literal",
                    "name": "module",
-                   "value": "PlainUserNameRenderer.react",
+                   "value": "MarkdownUserNameRenderer.react",
                    "type": "String"
                  }
                ],
                "storageKey": "__module_component"
              }

In this example, we see that our selections are similar, but we're swapping the order of MarkdownUserNameRenderer_name with PlainUserNameRenderer_name. See also: https://github.com/facebook/relay/blob/1bd6aa3491238819e5f089357ffcaf141c09156b/packages/relay-compiler/codegen/__tests__/fixtures/compileRelayArtifacts/query-with-match-directive.graphql#L10-L12

Here's another failure from Node 11:

@@ -32,10 +32,23 @@
          "concreteType": null,
          "plural": false,
          "selections": [
            {
              "kind": "InlineFragment",
+             "type": "NonNode",
+             "selections": [
+               {
+                 "kind": "ScalarField",
+                 "alias": null,
+                 "name": "name",
+                 "args": null,
+                 "storageKey": null
+               }
+             ]
+           },
+           {
+             "kind": "InlineFragment",
              "type": "Story",
              "selections": [
                {
                  "kind": "ScalarField",
                  "alias": null,
@@ -58,23 +71,10 @@
                      "name": "text",
                      "args": null,
                      "storageKey": null
                    }
                  ]
-               }
-             ]
-           },
-           {
-             "kind": "InlineFragment",
-             "type": "NonNode",
-             "selections": [
-               {
-                 "kind": "ScalarField",
-                 "alias": null,
-                 "name": "name",
-                 "args": null,
-                 "storageKey": null
                }
              ]
            }
          ]
        }
@@ -105,10 +105,23 @@
              "kind": "ScalarField",
              "alias": null,
              "name": "id",
              "args": null,
              "storageKey": null
+           },
+           {
+             "kind": "InlineFragment",
+             "type": "NonNode",
+             "selections": [
+               {
+                 "kind": "ScalarField",
+                 "alias": null,
+                 "name": "name",
+                 "args": null,
+                 "storageKey": null
+               }
+             ]
            },
            {
              "kind": "InlineFragment",
              "type": "Story",
              "selections": [
@@ -127,23 +140,10 @@
                      "name": "text",
                      "args": null,
                      "storageKey": null
                    }
                  ]
-               }
-             ]
-           },
-           {
-             "kind": "InlineFragment",
-             "type": "NonNode",
-             "selections": [
-               {
-                 "kind": "ScalarField",
-                 "alias": null,
-                 "name": "name",
-                 "args": null,
-                 "storageKey": null
                }
              ]
            }
          ]
        }

This time we notice that we're swapping the order of the selection of our types, selecting NonNode's selections before Story's selections. See also: https://github.com/facebook/relay/blob/1bd6aa3491238819e5f089357ffcaf141c09156b/packages/relay-compiler/codegen/__tests__/fixtures/compileRelayArtifacts/unions.graphql#L3-L11

It seems like we'll need to sort _everything_ to ensure this is completely stable.

Additionaly, is there more context into why we want to sort InlineFragments and Conditions after other selections? The git blame for this line isn't super helpful: https://github.com/facebook/relay/blob/1bd6aa3491238819e5f089357ffcaf141c09156b/packages/relay-compiler/transforms/SkipRedundantNodesTransform.js#L205

[kassens]

@eliperkins I was confused why we needed this as well at first, but then noticed the comment in the header:

https://github.com/facebook/relay/blob/40c8c915f768d9fa5b0d27449fb52498fc0e5907/packages/relay-compiler/transforms/SkipRedundantNodesTransform.js#L135-L140

I think what we can do here:
https://github.com/facebook/relay/blob/40c8c915f768d9fa5b0d27449fb52498fc0e5907/packages/relay-compiler/transforms/SkipRedundantNodesTransform.js#L152
is instead of .sort() is to iterate over node.selections twice: first for everything else, then for the Condition/InlineFragment nodes.

[josephsavona]

instead of .sort() is to iterate over node.selections twice: first for everything else, then for the Condition/InlineFragment nodes.

I think what we'd want is to iterate once over just ScalarFields/LinkedFields, and then again over everything else - fields are known to be fetched at a given point in a query, whereas the other nodes indicate that sub-selections are only evaluated conditionally (InlineFragment is really a type condition, Match is a wrapper around a set of type conditions, Defer/Stream have an if condition, etc).

[eliperkins]

I think what we'd want is to iterate once over just ScalarFields/LinkedFields, and then again over everything else

Do you mean to sort ScalarFields and LinkedFields _before_ everything else, rather than InlineFragments and Conditions _after_ everything else?

[eliperkins]

I figured it might be easier to discuss this over a PR, so I put one up at #2681. 🤷‍♂️

[josephsavona]

Yes, we should process fields before other selection types (whether via two iterations or a sort and a single iteration: two iterations would be faster on average). I think that this is different than the current logic, but it matches the original intent of this code (i remember bc I wrote it ;-)