facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.38k stars 1.82k forks source link

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

Open waynezhang1995 opened 2 weeks ago

waynezhang1995 commented 2 weeks ago

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 2 weeks ago

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 2 weeks ago

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

captbaritone commented 2 weeks ago

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: https://github.com/facebook/relay/issues/4772

captbaritone commented 2 weeks ago

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 2 weeks ago

Drew is on holiday, but I submitted a fix https://github.com/facebook/relay/pull/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 1 week ago

@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 1 week ago

@waynezhang1995 it was fixed in this commit https://github.com/facebook/relay/commit/4782743b8b0f2bc7a0cc133b92628e92e0ef57ac

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