[relay-compiler] Fix type generation of nullable types in TypeScript

[tobias-tengler]

Closes: #4379

Release Notes

Previously, the relay-compiler would generate a union of the actual type and null (T | null) for nullable fields when targeting TypeScript. This was not correct, since the Relay store can also return undefined in the case of missing data. This version now produces a union of the actual type, null and undefined (T | null | undefined).
Consequently you now also have to check both the null and undefined case, before accessing a field's value:

const data = useLazyLoadQuery(/* ... */);

// Notice the `!=` instead of `!==` to check against null and undefined
if (data.nullableField != null)
{
  // Safely access data.nullableField
}

Since this is a pretty big change, we're offering the typescriptExcludeUndefinedFromNullableUnion feature flag in the relay-compiler config to keep the old type generation behavior for now:

{
  "language": "typescript",
  "typescriptExcludeUndefinedFromNullableUnion": true
}

Just be aware that we will remove this feature flag in a future release and that you might run into an unexpected undefined, if you're not checking for undefined before accessing a field - especially if it's a client-only field.

[captbaritone]

Thanks!!

My guess is that this is going to be potentially difficult for folks to upgrade to. To help support their migration, would you add a compiler feature flag that gates this behavior, with the correct behavior being the default? We can plan to have at least one release where this change is opt-in before we remove the flag.

[tobias-tengler]

@captbaritone does it has to be a feature flag or can the switch also be on the TypegenConfig? This is already being passed to the TypescriptPrinter, so the change would be less disruptive to make.

[captbaritone]

Yeah, that's fine. Could you also do a writeup about this change that we can include in the next release notes? Ideally something that covers:

1. Values can be undefined in the case of missing data.
2. This has always been true, the types were wrong
3. We recommend performing null checks using == null to cover both null and undefined
4. An example of setting the feature flag/typegen config in a config file to temporarily opt out
5. This feature flag/option is temporary and will be dropped in a future version. Its intended for migration purposes only.

[tobias-tengler]

Where should I do the writeup?

[captbaritone]

Where should I do the writeup?

Wish I had a better answer to this. For now, maybe just a comment here on the PR or in a linked Google Doc (or similar)

[tobias-tengler]

@captbaritone Sorry for taking some time to get back to this - I was away for a few days.
I've added the release notes to the PR description.

While looking over the code again, I noticed some other type generation issues regarding client schema extensions:

1. Non-Null client schema extensions are typed as T, while they should be T | undefined (optional) in both Flow and TypeScript
2. Client schema extensions are included as optional on the rawResponse. Why are the client-only fields even included in the rawResponse?

I tried to tackle 1, but I lack the understanding of what conditional is in the code-base and am afraid to change something there. Could you maybe take a look at this?

[facebook-github-bot]

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

[facebook-github-bot]

@captbaritone merged this pull request in cc47bc9.

[ernieturner]

@tobias-tengler @captbaritone I was trying to upgrade a project of mine to Relay 15 and came across this addition. I'm not quite ready to tackle this change and update all my code, so I attempted to use the typescript_exclude_undefined_from_nullable_union flag to disable this, but I don't think it's actually possible to enable this flag for single project configurations. Or am I missing something?

In the config.rs file the ConfigFileProject (which is used in the Relay config when configuring multiple projects in a single config) flattens out the typegen_config: TypegenConfig which is where this new config was added, but that doesn't appear to exist on the SingleProjectConfigFile which is what I'm using (I think).

Is this just an oversight and theres something I'm missing or do we need to update the SingleProjectConfigFile to let people set all of the various typegen flags?

[tobias-tengler]

@ernieturner Thanks for bringing this up! This was indeed a small oversight on my part 😅 Should be fixed with #4482

[gajus]

I don't fully understand the purpose of this change.

Unless I am missing something, this will just result in every reference to a nullable field either changing to ?? null or ?: string | null. Which situations is this designed to help solve and why would it become the default?

Just want to make sure I understand the rationale for this or if I am even correctly thinking about how to adopt this change. At the moment, it is just a bunch of changes like:

const suggestions = (
  <OneLinerSuggestions
-    accountRef={userAccountRef}
+    accountRef={userAccountRef ?? null}
    isOpen={isOpen}
    onClose={closePopover}
    onEdit={() => {
      onEdit();
      closePopover();
    }}
    onSelect={handleSelectSuggestion}
    title="One-Liner Suggestions"
  />
);

export const isUpfrontTransfer = (
  transaction: SupportedTransactionRelayData,
): transaction is UpFrontTransfer => {
  return (
    'upfrontPayment' in transaction &&
    transaction.upfrontPayment !== null &&
+    transaction.upfrontPayment !== undefined &&
    transaction.upfrontPayment.amountCents > 0 &&
    (transaction.__typename === 'ContraTransfer' ||
      transaction.__typename === 'StripeTransferV2')
  );
};

  );

  if (profile === null) return null;
+  if (profile === undefined) return null;

  return (
    <>
      <Helmet>
        <title>{`Opportunities offered by ${profile.firstName} ${profile.lastName}`}</title>
      </Helmet>

[tobias-tengler]

@gajus The reason I created this PR was this comment by one of the maintainers: #4370 (comment)

You don't have to duplicate all your if conditionals, just use == instead of ===, since undefined and null share the same semantic value, even though their types are different.

I think an example of where this change could help you catch bugs, is someone deleting a record from the store. This could result in your linked fields or arrays suddently containing an undefined value.

In theory this could also happen for non-nullable types, so in hindsight this should've maybe been solved in the store and not the type generation.

Maybe @captbaritone can provide some additional insights.

[gajus]

You don't have to duplicate all your if conditionals, just use == instead of ===, since undefined and null share the same semantic value, even though their types are different

That's inconvenient because we enforce strict checks across our codebase. So it is going to be either:

  );

  if (profile === null) return null;
+  if (profile === undefined) return null;

  return (
    <>
      <Helmet>
        <title>{`Opportunities offered by ${profile.firstName} ${profile.lastName}`}</title>
      </Helmet>

or

  );
-  if (profile === null) return null;
+  // eslint-disable-next-line no-eq-null, eqeqeq
+  if (profile == null) return null;

  return (
    <>
      <Helmet>
        <title>{`Opportunities offered by ${profile.firstName} ${profile.lastName}`}</title>
      </Helmet>

Neither of which is a desirable change.

This also means patching thousands of references in codebase, which is a major undertaking. We can probably write a script to automate most of it based on TypeScript suggestions, but my problem is that it makes the code less readable, i.e. goes back to this being undesirable change from maintenance perspective.

In theory this could also happen for non-nullable types, so in hindsight this should've maybe been solved in the store and not the type generation.

@captbaritone This really does feel like it should have been solved in the store and not the type generation.

[gajus]

@captbaritone any chance of reconsidering this?

[gajus]

@kassens @alunyov @SamChou19815 is it possible to get some follow up here?

[alunyov]

So, I think the rationale for this change is well described by @tobias-tengler - in Relay, null and undefined are different things. If the type system can help reflect the actual runtime behavior, it is a good change from the correctness perspective.

However, from a practical point of view, I agree that this change may break a lot of existing codebases, where, for various reasons, the handling of null/undefined values may be different. We also had to disable this flag for one of our internal projects that was using TypeScript. It is likely we won’t be updating that project to a different null handling in the near future, so the feature flag will probably stay.

@gajus, am I understanding correctly that your proposal is to revert this change (type updates) and handle this at runtime by always returning null values and never undefined?

[gajus]

That's correct. I'd propose to revert this. There is extremely little value for this to be exposed to the client. You could make it exposed behind a feature flag (the inverse of the current behavior), but even then it is very hard to see how this would benefit someone even if they are starting the project from the ground up.

[captbaritone]

I think there may be some confusion. The change here makes the type script generated types match the long-standing behavior of the runtime (and the generated Flow types). undefined is (and has been) a possible value in these locations forever. The types are just being updated to reflect this truth. This change does not introduce new undefined values, but just corrects the types to correctly represent the fact that these values can, in fact, be undefined.

[gajus]

I see. In which case, I would want to have an option to just return null for all the cases that can return null | undefined. We were quite happy all this time assuming that these are the only possible values.

We are currently working on a wrapper to do this for us, but I think that keeping the current behavior as the default will severely cripple new user adoption of Relay.

[ernieturner]

Doesn't the typescriptExcludeUndefinedFromNullableUnion: true flag in the Relay config give you that option to keep the return type as only nullable and not null | undefined? You shouldn't need to create your own wrapper for this as that flag should just revert the behavior back to Relay < 16. When we upgraded to Relay 16 we just enabled that flag and no other changes were necessary.

[gajus]

The release notes explicitly state that the feature flag is going to be removed in the future, so we need to proactively plan for it.

And also, the feature flag does not cover all the cases, e.g.

Even though "typescriptExcludeUndefinedFromNullableUnion": true, the above code still includes undefined as a possible return value.

[ernieturner]

Given this discussion it seems like it's reasonable to reverse course on the plan to remove the feature flag and just keep it around forever. I can't speak to the feasibility of that, but it seems like something that people should be allowed to opt into forever if they want.

Regarding the use case where you're still getting undefined in the union, that seems odd as we haven't had that problem come up once we enabled that feature flag. Maybe there's something different with how that fragment is defined? Not sure what might be happening there.

[captbaritone]

And also, the feature flag does not cover all the cases, e.g.

Interesting. Did version 16 return only null there? If so, can you open a separate issue?

[captbaritone]

I'm open to leaving the feature flag indefinitely. We are starting to look at alternate ways to handle missing data more explicitly, perhaps piggybacking on our explicit error handling efforts. So, there's a possibility that we'll have a fundamental solution which can allow us to avoid returning undefined in this positions (since they will turn into errors) at which point we'll be able to update the types to be T | null while still being accurate.

[captbaritone]

I'm not particularly concerned about the current default interfering with new adoptions of Relay. Handling both null and undefined as a single concept with == null should be acceptable for most users.

[captbaritone]

I've updated the release notes to clarify our thinking here: https://github.com/facebook/relay/releases/tag/v16.0.0

[alex-statsig]

And also, the feature flag does not cover all the cases, e.g.

Interesting. Did version 16 return only null there? If so, can you open a separate issue?

I'm not sure if this issue got opened, but I realized during some upgrades that @types/react-relay types useFragment to return KeyTypeData<TKey> | null | undefined in recent versions