Relay: [modern] @include and @skip directives didn't work

[DrBoria]

@include and @skip directives didn't work in relay-modern.

[josephsavona]

Hmm. These are implemented/tested and we're using them in production. Can you provide more information to help narrow down what might be happening?

• What version are you using (version number and classic/compat/modern)?
• What is the query you're executing and what are the variables?
• Does that match up with the query sent to the server and the response you receive?

[DrBoria]

In relay-modern version 1.0.0, in your todo-modern example (https://github.com/relayjs/relay-examples/tree/master/todo-modern) with last commit from april 27, when I use @skip or @include in queries they are still fetched. I mean, query from client goes to the server and includes all data, with @skip(if: true), too.
In relay classic, version 0.10 it works correct, but in modern this functionality broken.

[josephsavona]

Hmm. Relay Modern eliminates skip/include directives when the value is known statically, so it should never print a literal "@skip(if: true)".

If you forked Relay-examples, can you share a link to your fork and the commit with the added directive? What query is sent and what data do you get back from the server?

[robrichard]

For dynamic uses of @skip and @include, Relay Classic would remove that part of the query (since the query was constructed dynamically). Relay Modern does not do this since it does not know at compile time if the data is needed or not. Instead it will rely on the GraphQL server to ignore the skip'd part of the query. Maybe your GraphQL server is not fully spec compliant and is not correctly ignoring skip'd fields?

(apologies to github users skip and include for the mentions on this issue...)

[DrBoria]

Well, I'm talking about something like this.
https://github.com/facebook/relay/blob/9f74316918be1833861a4f326e535e8d5ab9de59/packages/relay-compiler/transforms/__tests__/fixtures/skip-unreachable-node-transform/removes-include-false.golden.txt
This functionality is still not work. I tried to add variables in query renderer and use @skip/@include and it didn't work.

[josephsavona]

@DRBoria the test file you linked to shows that these fields are removed correctly. Can you please provide an example input query, what was sent to the server, and the response you got back? It's easiest to debug if we have a specific failing example.

[josephsavona]

Sorry for the close/reopen, fat fingers on mobile.

Just a note that the test file is for only one transform. The reason that the one fragment reference is still there is that fragment argumentDefinitions are handled by a different transform: when all transforms are enabled the compiler would output an empty query for that case.

So having a concrete real life example would really help here! Thanks :-)

[DrBoria]

Ok, I forked todo-modern example, you can see changes in my fork (https://github.com/DrBoria/relay-examples/blob/master/todo-modern/js/components/TodoApp.js). I add @include directive in TodoApp component. And, because of new variables definition, I add @argumentDefinitions directive. And got exception - http://take.ms/vkfuQ

[sibelius]

try to use graphql.experimental

[DrBoria]

I don't know, what is graphql experimental, but if I can't use @skip or @include directives in relay 1.0.0, it looks like issue.

[DrBoria]

May be there is another new syntax of using it? (@skip, @include)

[josephsavona]

Can you try adding @include without using @argumentDefinitions (sounds like that's what you did originally - using a global variable defined on the query)? Again, we know this feature works in general so having a concrete failing example would be very useful to diagnose. You might be hitting an edge case but it's hard to guess what that might be. Thanks for your patience!

[DrBoria]

@josephsavona well, I can try many different things. But using @skip(if: true) it didn't work and usig @argumentdefinitions didn't work;
So, can you provide correct variant how to use it? Cause I didn't found it in your api. There was case only for relay classic.

[josephsavona]

@DrBoria Hmm. Without a concrete failing example, this is really hard to debug. Here's an example of using the directives that I just confirmed works:

// pages/index.js
import ReactDOM from 'react-dom';
import {graphql, QueryRenderer} from 'react-relay';
ReactDOM.render(
  <QueryRenderer
    environment={/* your environment here */}
    query={graphql`
        query pagesQuery($showName: Boolean = false) {
          viewer {
             actor {
                id
                name @include(if: $showName) # will be undefined unless you pass {showName: true} in variables
             }
           }
         }
      `}
       variables={{showName: true}}
       render={({error, props}) => {
         if (props) {
           return <div>{props.viewer.actor.name}</div>; // undefined when showName:false
         } else if (error) {
           return <div>Error: {error.message}</div>;
         } else {
           return <div>Loading</div>;
         }
       }}
     />
);

[DrBoria]

Well, it's example of how to use inside queryRenderer. And how to use this directives inside fragments?
Like here, for example?
export default createFragmentContainer(TodoApp, { viewer: graphql' fragment TodoApp_viewer on User{ id, totalCount @include(if: $count), ...TodoListFooter_viewer, ...TodoList_viewer, } ', });

[DrBoria]

And you can see in my fork, in todo-modern example, I repeat your code for fragment and plain string in query, and it didn't work. Could you please verify and if i'm wrong provide correct variant how to use it?

[DrBoria]

Well, I should add, p.s. to my last comment. It still send line with @include and I get responce with it, so, perhaps it is issue with server, not client side. But I still can't use @skip and @include directives inside fragments. What api should I use? Could you please provide?

[josephsavona]

Sure, as with the query example this follows standard GraphQL syntax:

graphql`
  fragment Foo on SomeType {
    id
    name @include(if: $cond)
  }
`
// note that the including query would define the variable, per GraphQL spec:
graphql`
  query SomeQuery($cond: Boolean = true) {
    someField {
      ...Foo
    }
  }
`

For Relay's experimental fragment local variables you could use:

graphql.experimental`
  fragment Foo on SomeType
  @argumentDefinitions(
    cond: {type: "Boolean", defaultValue: true}
  ) {
    id
    name @include(if: $cond)
  }
`

[josephsavona]

@DrBoria guessing that worked?

[DrBoria]

Thanks, looks like it's correct variant. Probably, it will be good, if you'll add this in relay-modern description here https://facebook.github.io/relay/docs/fragment-container.html

[hisapy]

Hey guys

I'm having a similar issue. Neither @skip or @include are not working in my case. The following is my query:

<QueryRenderer
    environment={Auth.getEnvironment()}
    variables={{id: id || '', noId: !id}}
    query={graphql`
      query CompanyFormQuery($id: ID!, $noId: Boolean) {
        node(id: $id) @skip(if: $noId){
          ...CompanyForm_company
        }
        viewer {
          ...CompanyForm_viewer
        }
      }
    `}
  render={...}
/>

The request always includes the node query. Is it possible what I'm trying to achieve here?

[robrichard]

@hisapy in your example you should see the query get sent to your GraphQL server with the @skip directive in there. Your GraphQL server should understand the directive and remove it from the results. Relay Classic used to remove skipped fields from the query automatically, but Relay Modern relies on your GraphQL server supporting those directives. Which GraphQL server library are you using? Does it have support for @skip and @include?

[hisapy]

Hi @robrichard

I'm using Absinthe, GraphQL implementation for Elixir. The server supports it but I was hoping that fields _tagged_ with these directive were skipped from the query in Relay. I wonder why this is not possible since Relay now uses _new_ directives like @arguments or @connection. I'm not sure but I think the compiler only has to consider a binary (true/false) condition to compile the possible queries. On the other hand, perhaps this is left over because if you don't want to send the whole query string you can setup persisted queries but unfortunately my team didn't get to that point yet.

[josephsavona]

Relay Modern uses static queries and does not modify queries at runtime. At build time, the compiler sees the directive but doesn't know the value of the condition yet ($noId), so it has to leave that field in the query. So in this case the field is sent to the server, but if the server is spec compliant it should not return a value for it.

Note that with @arguments it is sometimes possible for Relay to know an include/skip value statically in which case it can be pruned.

[MatthewHerbst]

Should the server be attempting to validate parts of queries where include is false?

Ex:

contacts(
  # This argument is a String! in our schema
  loggedInUserUid: $uid
) @include(if: $profileIsLoggedIn) {
  ...
}

In the above scenario, if profileIsLoggedIn is false, then the uid will always be null since we have no profile to pull a uid from. However, our server is failing query validation because the uid field is non-nullable. We were expecting the include to skip the argument validation if that part of the query isn't needed. Should it be? If not, is there a recommended best practice on how to do this? Split the query up into multiple queries, or make the uid field nullable?

[sibelius]

make $uid nullable works fine

[MatthewHerbst]

make $uid nullable works fine

@sibelius That's by far the simplest solution, agreed. However, it does feel like it significantly harms schema design, since the contacts query cannot work without a valid uid. Does the server really need to validate those params if the include is false?

[sibelius]

this could be a great issue on graphql-js I guess