Generate valid types when the same field is selected by both conditional and unconditional fragments

[vhfmag]

This fixes #4914 — check the original issue for details about the bug this tries to solve. Here's how the types generated differ before and after this change:

	Typescript	Flow
Before	export type AppQuery$rawResponse = { readonly account: { readonly id: string; readonly name: string; } | null | undefined; readonly account: { readonly email: string; } | null | undefined; };	export type AppQuery$rawResponse = {| +me: ?{| +firstName: ?string, +id: string, |}, +me?: ?{| +lastName: ?string, |}, |};
After	export type AppQuery$rawResponse = { readonly account: { readonly id: string; readonly name: string; readonly email?: string; } | null | undefined; };	export type AppQuery$rawResponse = {| +me: ?{| +firstName: ?string, +id: string, +lastName?: ?string, |}, |};

More specifically, if both conditional and unconditional selections are present for a given linked field, it emits a single non-optional prop, but handles fields from the conditional selection as if they were conditional themselves (AKA ...Fragment @include(if: $condition) + fragment Fragment on Type { field } behaves the same as ...Fragment + fragment Fragment on Type { field @include(if: $condition) }.

[facebook-github-bot]

Hi @vhfmag!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

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

[facebook-github-bot]

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

[evanyeung]

Hi @vhfmag, sorry this has taken a while to get merged! It turns out that you exposed another issue in our typegen where we allow multiple keys to be generated in a single object more generally. This turns out to be valid Flow code so hasn't been noticed so far and we'll have to fix it before merging. Unfortunately, we're making a release today and this won't make the cut. We'll prioritize this to be included in our next release in 6 weeks.

[vhfmag]

Hi @evanyeung! No worries, thanks for the update and for looking into this

[facebook-github-bot]

@evanyeung merged this pull request in a3ddd58.

[MarceloPrado]

@evanyeung any issues with this PR? we noticed it was reverted 🤔

[evanyeung]

Yeah, I had to revert it as we are seeing an unexpected change in one of the types generated in our codebase. I'm digging into it and hope to re-land this still when I have an understanding on why the type was changed.

[evanyeung]

I found some time to look into this and have a repro of where the bug. Using the .extend in visit.rs may produce the wrong result if there are shared field keys among the fields being merged. For example,

query AppQuery($showLastName: Boolean!) @raw_response_type {
  ...AppFragment
  ...AppConditionalFragment
}

fragment AppFragment on Query {
  me {
    actor {
      firstName
    }
  }
}

fragment AppConditionalFragment on Query {
  me @include(if: $showLastName) {
    actor {
      lastName
    }
  }
}

incorrectly overwrites the firstname type with the lastname one without merging them. I think the field merging logic from raw_response_make_prop will have to be applied recursively without just extending the record fields directly like this.

Is this something you'd be able to look in to @vhfmag?

[vhfmag]

of course! I'll let you know once I have something ready @evanyeung

[vhfmag]

@evanyeung sorry for the delay, but hopefully the latest commit fixes the issue — I've modified the conditional-and-unconditional-fragments tests to include nested linked fields to catch the bug in tests and fixed raw_response_make_prop by making use of merge_selection_maps

[vhfmag]

hey @evanyeung! just wanted to make sure you saw the message above so that we can include this fix in the next release if everything is working correctly now 😬

[evanyeung]

Thanks for the update! I think there is still a small bug in the generated types here

[evanyeung]

I think this is technically the incorrect type. Since the actor field is included non-conditionally in the AppFragment fragment, this type should be unconditional. We really only want the subfields from AppConditionalFragment to be optional. I think this is due to the actor in AppConditionalFragment getting passed the conditionality of the me field? But the non-conditional from the AppFragment should override it.

[vhfmag]

I'll look into it!

[vhfmag]

@evanyeung I think this is actually correct, because User.actor is typed as Actor (which is nullable). it would be incorrect if the key-prop pair was optional (actor?:). you can also see this is in line with the behavior elsewhere by looking at the type of the actor field in the fragments, for which the field isn't conditional but is still nullable

[vhfmag]

it's arguably a bit less confusing in the TS version, where prop optionality (?:) is very easily distinguishable from value nullability (: { ... } | null | undefined for TS, : ?{ ... } for Flow)

[vhfmag]

@evanyeung gentle bump on this

[evanyeung]

Ah, I think you're right. Sorry for so many delays on this. I should be able to take another look next week.

I was locally testing this and I think the issue came in Flow where we currently have another bug where it's actually possible to generate two fields on an object type with the same name. When trying to collapse those fields into one field, I had to edit the code you most recently added that recursively merges the field types. That may be where I was seeing the wrong types on the Flow side and I incorrectly attributed them to the types in the test here.

[vhfmag]

ah, gotcha!

[facebook-github-bot]

@evanyeung has imported this pull request. If you are a Meta employee, you can view this in D80381458.

[jantimon]

We just ran into the same issue Stackblitz Reproduction

My Question is - shouldn't it rather be a union instead of a merged type?

query AppQuery($showLastName: Boolean!) @raw_response_type {
  ...AppFragment
  ...AppConditionalFragment
}

fragment AppFragment on Query {
  me {
    actor {
      firstName
    }
  }
}

fragment AppConditionalFragment on Query {
  me @include(if: $showLastName) {
    actor {
      lastName
    }
  }
}

I believe for this example actor should be { firstName: string } | { lastName: string } or is there a reason why merging the type is better?

[vhfmag]

@jantimon In your example, there are two shapes actor can take:

• If showLastName is true, { firstName: string; lastName: string }
• Otherwise, { firstName: string }

So if it were to be a union, it should be { firstName: string } | { firstName: string; lastName: string }, not { firstName: string } | { lastName: string } (in other words, firstName, the non-conditional selection, must be present in both variants).

That said, while a union might be a more accurate representation in some cases, it's also less convenient. For example, imagine this setup:

query AppQuery($include: Boolean!) @raw_response_type {
  ...AppFragment
  ...AppConditionalFragment
}

fragment AppFragment on Query {
  me {
    actor {
      firstName
    }
  }
}

fragment AppConditionalFragment on Query {
  me @include(if: $include) {
    actor {
      lastName
      age
    }
  }
}

In this case, a union ({ firstName: string } | { firstName: string; lastName: string; age: number }) is more accurate than a merged type with optional fields ({ firstName: string; lastName?: string; age?: number }) because it encodes the fact that if lastName is present, so will be age (and vice versa).

But that comes at the cost of complexity: if we have two conditional fragments, the union would have 4 variants (fragment 1 and 2 skipped; fragment 1 included and 2 skipped; fragment 2 included and 1 skipped; fragment 1 and 2 included); more generally, if we have N conditional fragments, the union would have 2^N variants — which can get out of hand quickly and lead to TS performance issues, cryptic error messages, etc.

Additionally, it requires (IMHO) clunky code to look for the conditional fields:

// with optional fields, it's just typical JS/TS code
const name = data.lastName ? `${data.firstName} ${data.lastName}` : data.firstName;
// with a union type, we can't look for `data.lastName` without narrowing the type first,
// because it doesn't exist in one of the variants
const name = "lastName" in data ? `${data.firstName} ${data.lastName}` : data.firstName;