facebook/relay

[Relay 18] Relay Resolver return type not getting derived from resolver function

Opened this issue · 9 comments

waynezhang1995 commented

Creating issues for Relay

Questions regarding how to use Relay and/or GraphQL

We are experimenting with Relay 18 and have noticed that the return type of a Relay resolver with RelayResolverValue is no longer being derived in the generated TypeScript type definition

For example:
Given the following relay resolver implementation

type ContactInfo = {
  address: string;
  phone: string;
};

/**
 * @RelayResolver PersonalDetails.contactInfo: RelayResolverValue
 */
export function contactInfo(personalDetailsRef: RelayResolversPostalCodeFragment$key): ContactInfo {
  const data = readFragment(
      graphql`
        fragment RelayResolversPostalCodeFragment on PersonalDetails {
          address
          phone
        }
      `,
      personalDetailsRef,
    );

  // a very dummy example
  return {...data}
}

In the generated relay artifact that references the contactInfo

import { ConcreteRequest, Query } from 'relay-runtime';
import { FragmentRefs } from "relay-runtime";

// This is missing in Relay 18
// import { contactInfo as personalDetailsContactInfoResolverType } from "../RelayResolvers";

export type RelayResolversComplexRequiredThrowQuery$variables = Record<PropertyKey, never>;
export type RelayResolversComplexRequiredThrowQuery$data = {
  readonly viewer: {
    readonly userProperties: {
      readonly personalDetails: {
        readonly contactInfo: ReturnType<typeof personalDetailsContactInfoResolverType> | null;
      } | null;
    };
  };
};

Relay used to derive the return type import { contactInfo as personalDetailsContactInfoResolverType } from "../RelayResolvers";, but this is now missing in Relay 18

Appreciate any insights. Thanks

Reporting issues with Relay

We will be using GitHub Issues for our public bugs. We will keep a close eye on this and try to make it clear when we have an internal fix in progress. Before filing a new issue, make sure an issue for your problem doesn't already exist.

The best way to get your bug fixed is to provide a reduced test case. To make reproduction simple for you, and set-up simple for Relay's maintainers, you can use Glitch:
https://glitch.com/edit/#!/remix/relay-starter-kit

You can also provide a public repository with a runnable example.

Security bugs

Facebook has a bounty program for the safe disclosure of security bugs. With that in mind, please do not file public issues; go through the process outlined on that page.

waynezhang1995 commented

We are on the following:

 "react-relay": "^18.0.0",
 "relay-runtime": "^18.0.0",
"relay-compiler": "^18.0.0"

 "@types/react-relay": "^16.0.6",
"@types/relay-runtime": "^17.0.4",

Could the issue be due to the @types being outdated?"

captbaritone commented

Thanks for the report! Let me see if I can repro/fix.

  • ❤️1
captbaritone commented

I think this relates to these lines:

We actually have a few integration tests which currently show this broken behavior. For example:

@drewatk do you want to take a look at this?

See also: #4772

captbaritone commented

Note: Removing those == Flow test seems to add the required imports, but also adds some unused imports which I know can be an issue for typechecking.

Markionium commented

Drew is on holiday, but I submitted a fix #4791.

I ran into this issue yesterday and realised we made a mistake by removing it, thinking it was always unused.

Removed ifs around the import statement but left the TODOs as there is still work to do.

@captbaritone might make sense to have a Typescript validation check on the generated files so CI can catch this? (That is after we fix type checking those files and remove the @ts-nocheck.)

waynezhang1995 commented

@captbaritone any updates on this issue, please? Are we good to merge the fix? It is currently blocking us from upgrading to Relay 18.

Markionium commented

@waynezhang1995 it was fixed in this commit
4782743

@captbaritone can hopefully release a new version that includes it.

  • 🎉1
Markionium commented

@waynezhang1995 please check 18.1.0, that should have solved the issue.

ernieturner commented

@Markionium @captbaritone

So the 18.1.0 release only partially fixes this issue, specifically it only fixes it when querying a Relay resolver field in a fragment, but it is still broken if the field is selected as part of a query. I think something got missed in the original PR, #4791 and the followup change where we only partially removed the TypegenLanguage::Flow check. Specifically, in the original PR in the write.ts file, both if checks were removed around the write_relay_resolver_imports call, which is correct. However, that PR had some weirdness during in the internal import and so only part of the change was actually changed. The resulting change only removed the if check for the write_fragment_type_exports_section but omitted the change in the write_operation_type_exports_section function.

I'm going to try and find some time today to get a PR up for this change and see if I can't get some unit tests added as well.