Ignore value nullability

[brabeji]

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix

• What is the current behavior? (You can also link to an open issue here)

The link validates the response data but does not account for @skip and @include directives.

• What is the new behavior (if this is a feature change)?

The link now disregards result value type nullability. We should just convert scalars and don't bother with the nullability validation (apollo client does this). This fixes the link raising errors for non-nullable fields even when they're skipped with @skip directive. This should also take care of @include directive, but this is yet to be tested.

[eturino]

@brabeji LGTM but lint fails in CI. Could you fix that? I probably will change that lint rule later tbh…

[brabeji]

@eturino Definitely, I was just probing if this is a valid change. Will fix asap

[eturino]

I am thinking about this change a bit more. Indeed this link should not be responsible to ensure null/non-nulls. While your change fixes it for that case, it wouldn't do it on a nested field.

Maybe what we can do is to change the behaviour of ensureNullableType

apollo-link-scalars/src/lib/ensure-nullable-type.ts

Line 12 in 0014120

export function ensureNullableType(

We call this at the beginning of treatValue(). What if instead of calling that, we call directly getNullableType()?

apollo-link-scalars/src/lib/parser.ts

Line 68 in 7499d45

const type = ensureNullableType(value, givenType, fieldNode);

We would have to adapt this test

apollo-link-scalars/src/__tests__/validate-enums-from-query.spec.ts

Line 188 in 1f8fbf5

describe("null values on non-null field", () => {

What do you think @brabeji ?

[eturino]

I've added an issue #29 to cover the removal of the nullability checks.

[brabeji]

What if instead of calling that, we call directly getNullableType()?

Yes, that makes sense. I wasn't digging deep enough!

I have some issues with the resulting TS type, which I was unable to solve yet, though. See https://github.com/eturino/apollo-link-scalars/pull/28/files#diff-a5b54a3eee29198834db8832951d216eR69

I've "adapted" 🔪 the test and broken coverage. I'll fix that later today.

[eturino]

I see. I'll have a look this evening and see if we can avoid that

[brabeji]

I've compared coverage results with master and I can't see any increase in uncovered lines count. Does it look like -0.2% might be expected change when removing code?

[eturino]

@brabeji
The issue with using getNullableType() directly is because of the typing definition. We can change the ensureNullableType() function to be:

export function ensureNullableType(
  type: GraphQLOutputType | GraphQLInputType
): GraphQLNullableType {
  return isNonNullType(type) ? type.ofType : type;
}

and that should work fine. If I am not mistaken, that is what getNullableType() does anyway.

btw, re the percentage of lines tested, do not worry. The deleted file was fully tested so taking it out of the equation will always reduce the overall % of tested lines.

[eturino]

@all-contributors please add @brabeji for bug, tests, code and idea

[allcontributors]

@eturino

I've put up a pull request to add @brabeji! 🎉