make new function exposed from the enricher so lookaheads can check and evaluate skips / includes

[trkohler]

Bug description

Even though fields inside the entities() field of a collection are @skip-ed as part of a query, they still make the calls to backends as if they are not skipped but just discards the data instead of returning it to the client.

Expected behavior

Backend systems should not be called in the case that the field it is required for is skipped.

Current behavior

Backends are called even if the field is skipped

[trkohler]

current problem

this test with snapshot is not passing src/__tests__/resolve-info.test.ts:902

there is query

query {
          node(id: "root") {
            id
            ... on Image {
              width
            }
            ... on DocumentElement {
              __typename
              ... on Image {
                url
                tags {
                  name
                }
              }
              ... on Media {
                tags {
                  id
                }
              }
            }
          }
        }

and I receive that my result contains property name but apparently it should not

"tags": Object {
            "Tag": Object {
              "id": Object {
                Symbol(LeafFieldSymbol): true,
              },
    -         "name": Object {
    -           Symbol(LeafFieldSymbol): true,
    -         },

but we query property name on tags, aren't we? why we shouldn't have this property in snapshot?

UPD: test is correct. my debug document (access only for zalandoers)

[oporkka]

and I receive that my result contains property name but apparently it should not

"tags": Object {
            "Tag": Object {
              "id": Object {
                Symbol(LeafFieldSymbol): true,
              },
    -         "name": Object {
    -           Symbol(LeafFieldSymbol): true,
    -         },

but we query property name on tags, aren't we? why we shouldn't have this property in snapshot?

I re-read the comment now, and realised it is other way around. The snapshot is expecting name to be there, but the result returned by infNode.fieldExpansion does not have it. Would that mean that the PR caused some sort of regression which the tests are now catching?

[trkohler]

yes, that's why it certainly must not be merged and I need to find a place where it's happening because my additions were not meant to modify query response tree.

[ponimas]

I have 0 ideas how, but these lines break objects merging and lead to the failing test in this PR.

[oporkka]

The object merging breaks because we set here a prototype for the object, but lodash mergeWith is expecting objects to be plain objects to function, and it does not know how to merge objects that have non null prototypes.

See https://lodash.com/docs/4.17.15#merge

Array and plain object properties are merged recursively. Other objects and value types are overridden by assignment.

Basically because we are not having plain objects anymore, in this code https://github.com/lodash/lodash/blob/2da024c3b4f9947a48517639de7560457cd4ec6c/.internal/baseMergeDeep.js#L69-L89 we skip this merge part, and just override the value, as stated above

[oporkka]

Our customizer for mergeWith is only customizing the leaf nodes, and returns undefined for other nodes, i.e. then normal lodash merge is used.

One way to deal with this would be to extend the customizer function to also merge our new objects that have prototype

graphql-jit/src/resolve-info.ts

Lines 378 to 393 in 9de816e

	mergeWith(obj, src, (objValue, srcValue): LeafField | undefined => {
	if (isLeafField(objValue)) {
	if (isLeafField(srcValue)) {
	return {
	...objValue,
	...srcValue
	};
	}
	return objValue;
	} else if (isLeafField(srcValue)) {
	return srcValue;
	}
	return undefined;
	});

[boopathi]

The typename "ShouldIncludeVariables" should be ShouldIncludeParams. Otherwise it would read variables.variables[variableName].

[trkohler]

renamed.

[boopathi]

Why do we need this separate type?

[trkohler]

initially it was because root doesn't have ShouldIncludeExtension since root is always included.

[boopathi]

• misbehaving spreading fragments #166

Update: The fix to the above issue will have conflicts with this PR. So, I suggest that we hold this PR until we fix the bug in #166 - the fix to #166 would likely remove the __internalShouldInclude which this PR relies on.

[trkohler]

current todo / problems

• exposing function in new attribute would be reflected in snapshots. need to figure out this
• tests are very simple now... can be much better [need ideas of test cases. would appreciate suggestions]
• address comments
• coverage increase (???)
  408,443 src/resolve-info.ts