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

Missing type definition in assignable fragment generated file in Typescript #4555

Open janicduplessis opened 9 months ago

janicduplessis commented 9 months ago

The relay compiler generates invalid type for assignable fragments in Typescript.

This is the generated code for the following fragment:

graphql`
  fragment SetSelectedMerchantProfile_assignable_profile on Profile
  @assignable {
    __typename
  }
`;
/**
 * @generated SignedSource<<f4c578c66d96d67f8e7e39b40b07ab4d>>
 * @lightSyntaxTransform
 * @nogrep
 */

/* tslint:disable */
/* eslint-disable */
// @ts-nocheck

import type { FragmentRefs } from "relay-runtime";

const node: any = {};

if (__DEV__) {
  (node as any).hash = "e25d274594f324a85a2b2e887789c902";
}

export default node;

module.exports.validate = function validate(value: {
  readonly __id: string;
  readonly __isSetSelectedMerchantProfile_assignable_profile?: string;
  readonly " $fragmentSpreads": SetSelectedMerchantProfile_assignable_profile$fragmentType;
}): false | {
  readonly __id: string;
  readonly __isSetSelectedMerchantProfile_assignable_profile: string;
  readonly " $fragmentSpreads": SetSelectedMerchantProfile_assignable_profile$fragmentType;
} {
  return value.__isSetSelectedMerchantProfile_assignable_profile != null ? value : false;
};

You can see that SetSelectedMerchantProfile_assignable_profile$fragmentType is never defined.

If I add

export type SetSelectedMerchantProfile_assignable_profile$fragmentType = {
  readonly __id: string;
  readonly __isSetSelectedMerchantProfile_assignable_profile: string;
  readonly " $fragmentSpreads": FragmentRefs<"SetSelectedMerchantProfile_assignable_profile">;
};

to the generated file then it type checks fine and I can also import the type SetSelectedMerchantProfile_assignable_profile$fragmentType for use in my app code.

tobias-tengler commented 9 months ago

I've added a fix in https://github.com/facebook/relay/pull/4556. Also going to add some more tests, since that area is lacking at the moment.

As with regular fragments, there is not dedicated type for the fragmentType. If you need the type reference you can infer it from the validate function parameter:

type AssignableType = Parameters<typeof validateFunction>[0];
janicduplessis commented 9 months ago

@tobias-tengler Thanks for fixing this! Shouldn’t assignable fragments also export something like FragmentName_field$key like regular fragments? Using validate type would work but is not ideal in my opinion.

tobias-tengler commented 9 months ago

What do you need the type for? As far as my understanding of the feature goes, the validate function is simply verifying that you're passing a suitable data structure and narrowing the type of that structure. The full type i.e. reader fragment should be on whatever you're passing into the validate function.

janicduplessis commented 9 months ago

I am looking to extract the mutation into its own function. Here's code I am trying to type:

import { commitLocalUpdate, graphql } from 'react-relay';
import { SetSelectedMerchantProfileUpdatableQuery } from './__generated__/SetSelectedMerchantProfileUpdatableQuery.graphql';

export const SetSelectedMerchantProfile_assignable_profile = graphql`
  fragment SetSelectedMerchantProfile_assignable_profile on Profile
  @assignable {
    __typename
  }
`;

export function setSelectedMerchantProfile({
  profile,
}: {
  profile: any; // <- TYPE THIS <-
}) {
  commitLocalUpdate((store) => {
    const { updatableData } =
      store.readUpdatableQuery<SetSelectedMerchantProfileUpdatableQuery>(
        graphql`
          query SetSelectedMerchantProfileUpdatableQuery @updatable {
            viewer {
              merchant {
                selectedProfile {
                  ...SetSelectedMerchantProfile_assignable_profile
                }
              }
            }
          }
        `,
        {},
      );

    if (updatableData.viewer.merchant != null) {
      updatableData.viewer.merchant.selectedProfile = profile;
    }
  });
}

I would expect to be able to do:

import { SetSelectedMerchantProfile_assignable_profile$key } from './__generated__/SetSelectedMerchantProfile_assignable_profile.graphql';

...

export function setSelectedMerchantProfile({
  profile,
}: {
  profile: SetSelectedMerchantProfile_assignable_profile$key
}) {

...
tobias-tengler commented 9 months ago

Seems like a valid use-case to me and I see the ugliness of the type situation there. I'm going to update my proposed PR to support this (hopefully tomorrow) :)

janicduplessis commented 9 months ago

I had a quick look and it seems we are purposefully not generating it here https://github.com/facebook/relay/blob/e1cefa5c1265896dbc0625344b82378c88c6b0b5/compiler/crates/relay-typegen/src/write.rs#L453, not sure if there was a reason, but adding it would be nice.