Relay: [relay-compiler] Exported fragment flow types aren't recursive

Created on 26 Jun 2017  路  9Comments  路  Source: facebook/relay

First, thanks for this amazing package. It's all we've always wanted. :)

When running relay-compiler against basic fragments, queries and mutations, I noticed that:

  • Fragments, queries and mutations aren't recursively typed. Which means that if a fragment references an other fragment, e.g. a Post fragment, that has a comments field with a Comment fragment inside, the comments field will be exported as a $ReadOnlyArray<?{| |}> and not as a $ReadOnlyArray with objects of the shape of the Comment fragment.

I'm assuming this is only because this is still a work-in-progress, and this feature is meant to be implemented.

Is there a roadmap regarding this feature ?
Could you give some starting points to make it easier to implement them ourselves, editing the code ?

Also, do you have an idea on how to work around this meanwhile ?


Example of not recursive fragments

./App.js

fragment App_comment on Comment {
    id
    author
    content
  }
fragment App_post on Post {
  id
  title
  comments {
    ...App_comment
  }
}



md5-602aaa84c5bf1bbfe00adc81eb591f26



...
export type App_comment = {|
  +id: string;
  +author: string;
  +content: string;
|};
...



md5-2432762326cdee88d32f75a9778f8072



...
export type App_post = {|
  +id: string;
  +title: string;
  +comments: $ReadOnlyArray<?{| |}>; // comments should be the same shape as App_comment
|};
...

Most helpful comment

We have some ideas to make the containers typed correctly. The idea is to generate a flow type like this:

export type App_post = {|
  +id: string;
  +title: string;
  +comments: $ReadOnlyArray<?{| |} & FragmentReference<App_comment>;
|};

FragmentReference<T> would not actually have any data directly because of the masking that Relay does. Instead, the Relay containers would be typed such that they unwrap the FragmentReference<T> into a plain T. I hope we'll got to this soon as it allows for a lot better typing, but it's a bit tricky to get the types right.

All 9 comments

We have some ideas to make the containers typed correctly. The idea is to generate a flow type like this:

export type App_post = {|
  +id: string;
  +title: string;
  +comments: $ReadOnlyArray<?{| |} & FragmentReference<App_comment>;
|};

FragmentReference<T> would not actually have any data directly because of the masking that Relay does. Instead, the Relay containers would be typed such that they unwrap the FragmentReference<T> into a plain T. I hope we'll got to this soon as it allows for a lot better typing, but it's a bit tricky to get the types right.

I've succeeded obtaining the desired result by hacking around the code, current way is by replacing the FragmentSpread node in the RelayCodeGenVisitor by an InlineFragment.

Example of working hack

EDIT 09/08/17: The hack worked on previous versions of the code. I've updated the links so they point to the corresponding older files. I hope you can inspire yourself from this to write the hack for current edited files.

./packages/relay-compiler/codegen/RelayFileWriter.js, replace by

const flowTypes = RelayFlowGenerator.generate(
  node,
  this._config.inputFieldWhiteListForFlow,
  nodes,
);

/packages/relay-compiler/core/RelayFlowGenerator.js

function generate(
  node: Root | Fragment,
  inputFieldWhiteList?: Array<string>,
  nodes, // add this argument
): string {
  ...
  // Make RelayCodeGenVisitor a function call, passing nodes
  const responseAST = RelayIRVisitor.visit(node, RelayCodeGenVisitor(nodes)); 
  ...
}

Make RelayCodeGenVisitor a function receiving nodes, returning the past object and add a FragmentSpread enter visitor

const RelayCodeGenVisitor = (nodes) => ({
  enter: {
    FragmentSpread: (node) => {
      const fragmentNode = nodes.find(n => n.kind === 'Fragment' && n.name === node.name);
      return {
        ...fragmentNode,
        kind: 'InlineFragment', // Replace by an inline fragment
        typeCondition: n.type,
      };
    },
   ... //others
});

@kassens Thanks for you reply, that sounds great. :)

In my case that would not fit all my use cases, as I plan to use graphql server-side (to query easily data from within my app, as for example inside cron jobs, or when transitioning from a REST api to a graphql endpoint). So to benefit from the strong typing, I need it to be typed as plain objects.

@yachaka I just tried your fix, but I think you maybe forgot to copy over a step. Above the first change where you pass in nodes into generate, where does nodes actually come from?

@cbranch101 Hey, I've noticed with your comments I had the links pointing to the latest versions of the files, and meanwhile obviously the code has been replaced.

I've updated my comment so the links point to the older versions of the code I was talking about, so you can understand how is the hack working.
I hope that can help you to apply it to the current version of the code.

@yachaka Yep, that was missing detail, thanks so much!

@kassens hi. any official updates on this?

@yachaka The issue in the original post is actually fixed by using @relay(mask: false). This solved the issue for me in a similar situation

Try this

fragment App_post on Post {
  id
  title
  comments {
    ...App_comment @relay(mask: false)
  }
}

This however has one drawback/bug. All properties directly under comments will be typed as nullable, even if they are set as required in the schema.

Hi everyone, flow type generation has been greatly improved for 1.5.0 (1.0.5-rc.1 has been released, and 1.5.0 is being released very soon), so I'm closing this for now; containers should be properly typed, and if you actually want to inline the shape for fragment spreads you can use @relay(mark: false) as @tanneess mentioned.

Going to close this, feel free to re-open if there are still any issues. Thanks!

Was this page helpful?
0 / 5 - 0 ratings