Do not output undefined for nullable fields within @throwOnFieldError and @catch

[tobias-tengler]

Since @throwOnFieldError or @catch will catch missing data errors, we can output nullable fields in the $data type as field: Type | null instead of field: Type | null | undefined (unless optional).

Closes #4971

Supersedes #4972

[tobias-tengler]

@captbaritone I've opted into always generating the nullable types in the raw response type without undefined (unless optional of course), but I'm wondering if this change should be rolled out together with the potentially new @rawResponseType directive that always outputs the semantic type instead?

[captbaritone]

@tobias-tengler Yeah, while I think you are correct that we should not allow optional types in raw response (since the server will never omit fields) in practice since they are currently typed that way it will be a major breaking change to update that behavior. At the very least we will need a feature flag, but I think bundling that flag (and therefor the PR) with a larger rethinking of raw response types makes more sense to me.

[tobias-tengler]

@captbaritone That's what I thought. I've updated the PR to only contain the change that undefined is omitted for nullable types within @throwOnFieldError and @catch.

[captbaritone]

I'm going to import this and see what breaking type changes is produces for us.

[facebook-github-bot]

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

[captbaritone]

Hmm. Looks like this creates many thousands of Flow issues internally. On common case seems to be people defining their own types to model a piece of a fragment and then assigning a bit of the Relay data into that type, which won't work.

Example: https://flow.org/try/#1N4Igxg9gdgZglgcxALlAIwIZoKYBsD6uEEAztvhgE6UYCe+JADpdhgCYowa5kA0I2KAFcAtiRQAXSkOz9sADwxgJ+NPTbYuQ3BMnTZA+Y2yU4IwRO4A6SFBIrGVDGM7c+h46fNRLuKxJIGWh8MeT0ZfhYlCStpHzNsFBAMIQkIEQwJODAQfiEyfBE4eWw2fDgofDBMsAALfAA3KjgsXGxxZC4eAw0G-GhcWn9aY3wWZldu-g1mbGqJUoBaCRHEzrcDEgBrbAk62kXhXFxJ923d-cPRHEpTgyEoMDaqZdW7vKgoOfaSKgOKpqmDA+d4gB5fMA-P6LCCMLLQbiLOoYCqgh6-GDYRYIXYLSgkRZkCR4jpddwPfJLZjpOBkO4AX34kA0SRWxgABAAVVYAMUo6QASng6OyALzs4AAHSg7Nl7IA9PL2QBqEjpbA8uB4NgAfmQ7PspigCAVSoA8rg2AaVm1eOyJLVaeyAO4QShbEjSuUqtXmTXa-WGiomgA+7KOuFN7IActhndbaG0rFzHSR2Wgoh7pfTpdK2dh2QBVMiUAAimgqpW5HPFUplcpEtAAylJg3rrUaENnc7AHso4NB7TQSDA3SIABQsXB0fXVjX8kRC6e0ACU+uLJnL8C+bDnEq9cpYEiElBlde93sbLc7+qndCsvo1WstB9lOag9NyIAaJhIA6gSQNAADFYABMABsACcVgAIwgPSQA

[tobias-tengler]

Interesting. At least with TypeScript it shouldn't be an issue, since the user defined type would have to be the same as Relay's current type (Type | null | undefined). Which means even if Relay is making the type narrower (Type | null), what we're assigning to is still broader. Playground
I've ran the type generation on our TypeScript code base and didn't encounter any issue.

Regarding Flow: The example you've made doesn't seem to reflect my changes. In your example the original Relay type is somehow just string | undefined and doesn't include null, which doesn't make much sense to me:

- +someField?: string
+ +someField: string | null

But looking at the snapshots, the original Relay type of a nullable field should be string | null | undefined (if I read up on the ?Type syntax correctly):

- +someField: ?string,
+ +someField: string | null,

Do you know why your original types do not include null for a nullable field?

[captbaritone]

Hmm. You're right. I think my example is not quite representative. Let me try to get one that's a direct reduction of the issue I saw internally.

[tobias-tengler]

@captbaritone did you manage to find a better example?