variable value set via prop is not considered in fetch

[Gregoor]

Apparently it is currently possible to set variables via identically named props. The subsequent(?) request however does not consider that set value.

I saw that you already have an issue for this in your internal bug tracker (

relay/src/container/RelayContainer.js

Line 832 in d2f9ae5

* TODO: Stop allowing props to override variables, #7856288.

), but I thought this issue might be valuable for other people who run in the same problem.

i just renamed the variable, which worked fine for me.

[josephsavona]

For context, variables have to passed as as props to child components because of cases like this:

// Container
fragments: {
  user: () => Relay.QL`
    fragment on User {
      ${Picture.getFragment('pic', {size: 32})},
      ${Picture.getFragment('pic', {size: 128})},
    }
  `
// Component
render() {
  return <Picture pic={this.props.user} />; // does this use `size: 32` or `size: 128`?
}

But rendering <Picture pic={this.props.user} size={32} /> makes it clear which set of variables apply to the child.

That said, this is an area of the API that we'd like to clean up. One proposal is the following:

// Container
fragments: {
  user: () => Relay.QL`
    fragment on User {
      ${Picture.getFragment('pic', {size: 32})} @relay(alias: 'lilPic'), # give an explicit alias...
      ${Picture.getFragment('pic', {size: 128})}, @relay(alias: 'bigPic')
    }
  `
// Component
render() {
  return <Picture pic={this.props.user.lilPic} />; // ...and reference it here (no need to pass variables)
}

[fson]

@josephsavona Why not use the GraphQL syntax for aliases instead of the special @relay directive in this case?

fragments: {
  user: () => Relay.QL`
    fragment on User {
      lilPic: ${Picture.getFragment('pic', {size: 32})},
      bigPic: ${Picture.getFragment('pic', {size: 128})},
    }
  `

[josephsavona]

@fson That would be awesome - but that syntax is not supported for fragments. The above is equivalent to the following:

fragments: {
  user: () => Relay.QL`
    fragment on User {
      # invalid syntax
      lilPic: ...pictureFragmentSmall,
      bigPic: ...pictueFragmentLarge
    }
  `

[fson]

@josephsavona Oh, I see.

In the @relay(alias: 'bigPic') example, I assume Relay would convert it to something like this:

fragment on User {
  lilPic: profilePic($size1) {
    ... on Picture { ... },
  }
  bigPic: profilePic($size2) {
    ... on Picture { ... }
  }
}

Where is the name of the field (profilePic in this case) defined?

[josephsavona]

cc @schrockn @leebyron

[josephsavona]

@fson Relay can't convert to lilPic: profilePic ... because that assumes the fragment has one root field, which is rarely the case.

Per offline discussion with @leebyron and @dschafer we'll probably have to add support for fragment aliases in the language to make this seamless. This would use your earlier proposal, @fson:

# Relay products
fragment on User {
  foo: ${Child.getFragment('foo')},
}
# Raw GraphQL
fragment on User {
  foo: ...sub_0 # name generated by `relay-babel-plugin`
}

[fson]

Thanks for the clarification @josephsavona.

This seems like the most obvious syntax. However, wouldn't it also assume a fragment with just one field that it can alias? What would happen if you try to apply it to fragment with many fields?

[josephsavona]

@fson the response for aliased fragments would be an object:

# source fragment
var userFragment = Relay.QL`
  fragment on User {
    id,
    name,
  }
`;

Then use it:

# query
fragment on Foo {
  user: ${userFragment} # equivalent to `user: ...fragmentName`
}
# response
{
  user: {
    id: "...",
    name: "...",
  }
}

[fson]

If that's how it would work, how does that differ from this (currently supported) syntax?

fragment on Foo {
  user {
    ${userFragment}
  }
}

Maybe there is some more complex use case that I'm not seeing right now...

[josephsavona]

@fson the new syntax would create a new object, into which the results of the fragment are spread. The current syntax spreads the result into the outer object.

Old result:

{
  id: "...",
  name: "...",
}

New result:

{
  user: {  // <-- fragment results are nested inside a new object
    id: "...",
    name: "...",
  }
}

[fson]

Ah, I got it now 👍 Thanks.

[saikat]

Am I right in thinking that this issue is saying that if I have a Relay container with variables set up like this:

export default Relay.createContainer(CallViewer, {
  initialVariables: {
    callId: "testId"
  },

  fragments: {
    viewer: () => Relay.QL`
        call(id:$callId) {
          ${Call.getFragment('call')}
        }
      }
    `,
  },
});

And then I render it like this:

<CallViewer callId="anotherId" />

anotherId will override the value of callId but will not actually cause a re-fetch?

[taion]

You need to pass through the variable mapping with getFragments as well, e.g. ${CallViewer.getFragments('viewer', variables)}

[vladar]

@josephsavona Looks like we have to pass both - variable mapping and prop with same name to make this work as @taion mentions above.

My example:
I need to display different number of items in connection for different scenarios. Child fragment has variable commentsShown.

But if I only pass variable mapping (without props) - Relay fetches all items as expected, but truncates them to initial variable value on render.

If I only pass prop with same name - Relay won't fetch all items (subject of this issue).

If I pass both - commentsShown variable and commentsShown prop everything works as expected. But that seems odd for me.

Am I missing something?

[josephsavona]

@vladar yup! This is because you might use the same fragment twice with different variables. See my answer above: #309 (comment)

[netgusto]

Variables set via props are (silently) breaking the fetch indeed.

Is this supposed to work ?

If not, what's the proper way to fetch data using variables based on props owned by the parent ? I tried setting relay variables in the component constructor, but:

• I suspect this is an anti-pattern
• relay triggers a first fetch with initial values for variables before using the variables set in the component constructor

[taion]

You're doing it wrong - follow the pattern above and inject the variables in via the container as well as via props.

[netgusto]

@taion but these prop'd variables are known only in the component context, and not statically in the fragments section of the relay container (for instance, a piece of state handled by a component).

Is there something I'm missing ? I mean, fragment variables live in a static context, and I can't see how I would be able to access component runtime data within that static context. Could you explain me, please ?

[vladar]

@netgusto

You should define variable mapping when including nested fragment:

viewer: (variables) => Relay.QL`
  someField {
    ${NestedContainer.getFragment('someField', {variable1: 'my-constant', variable2: variables.myProp})}
  }
}
`,

Then when your component is rendered with custom myProp Relay will assign it to nested container variable (variable2 in this example). Note, that myProp must also be defined in initialVariables even if it's null.

[quazzie]

How to do this for nested structure ?

https://stackoverflow.com/questions/34255662/props-in-relay-preparevariables

I want to set expanded for some of the tree.
I have saved the expanded state and when i navigate i save this in sessionStorage, when i return i want to restore the expanded state of all the branches and leafs.

[josephsavona]

@quazzie Thanks for asking. I'm writing an answer to your question on Stack Overflow.

[netgusto]

@taion @vladar Thanks for you answers.

(react-relay v 0.6.0)

Still, the relay variables set in the parent (Freelance) are set in the nested component (FreelanceList; when console.loging this.props.relay.variables I can see them), but not used by relay in the query fragment defined by FreelanceList.

I'm puzzled, to say the least.

import React from 'react';
import Relay from 'react-relay';

class FreelanceList extends React.Component {

    static propTypes = {
        job: React.PropTypes.string.isRequired,
        city: React.PropTypes.string.isRequired,
        first: React.PropTypes.number.isRequired,
        viewer: React.PropTypes.object.isRequired,
        relay: React.PropTypes.object.isRequired
    };

    render() {
        return (
            <div>
                <h2>Freelances {this.props.job} à {this.props.city}</h2>
                <ul>
                {!!this.props.viewer.freelances && this.props.viewer.freelances.edges.map(edge => (
                    <li>{edge.node.firstname} {edge.node.job}</li>
                ))}
                </ul>
            </div>
        );
    }
}

FreelanceList = Relay.createContainer(FreelanceList, {
    initialVariables: { job: null, first: null },
    fragments: {
        viewer: () => Relay.QL`
            fragment on Viewer {
                freelances(job: $job, first: $first) {
                    edges {
                        node {
                            id
                            ... on Freelance {
                                firstname
                                lastname
                                job
                            }
                        }
                    }
                }
            }
        `
    }
});

class Freelances extends React.Component {

    static propTypes = {
        params: React.PropTypes.object.isRequired,
        location: React.PropTypes.object.isRequired,
        viewer: React.PropTypes.object.isRequired,
        relay: React.PropTypes.object.isRequired
    };

    render() {
        return (
            <FreelanceList viewer={this.props.viewer} job={this.props.params.job} city={'Paris'} first={10} />
        );
    }
}

Freelances = Relay.createContainer(Freelances, {
    fragments: {
        viewer: (variables) => Relay.QL`
            fragment on Viewer {
                ${FreelanceList.getFragment('viewer', variables)}
            }
        `
    }
});

[netgusto]

I've set up this playground that reproduces the problem: https://goo.gl/i7uuMt
On line 24, change "the static message" by $message : props are passed, variables are set, but no query results

[netgusto]

anyone ?

[vladar]

@netgusto Yeah, this variable mapping process is one of the most error-prone and unclear areas in Relay.

It is better to understand it's internals better to avoid pitfalls. You must realize that GraphQL query is prepared in static time - before components are rendered. Relay cannot know value of your props at this point in time. That is why it must be passed in as variable (route param for initial rendering and setVariables for subsequent renderings).

But in your example HelloRoute doesn't define any variable mappings and is rendered without any params. As a result GraphQL query is constructed with default values!

To make your example work you must 1) change your Route:

class HelloRoute extends Relay.Route {
  static routeName = 'Hello';  // A unique name
  static paramDefinitions = {
    message: {required: true}
  };
  static queries = {
    echoz: (Component, variables) => Relay.QL`
      query {
        Viewer {
          ${Component.getFragment('echoz', {message: variables.message})},
        }
      }
    `,
  };
}

2. Add your message as route param on render:

ReactDOM.render(
  <Relay.RootContainer
    Component={HelloApp}
    route={new HelloRoute({message: "my message from props"})}
  />,
  mountNode
);

3. As far as I understand Relay expects explicit mapping, so your HelloApp container must be also tweaked:

HelloApp = Relay.createContainer(HelloApp, {
  initialVariables: {message: null},
  fragments: {
    echoz: (variables) => Relay.QL`
      fragment on Viewer {
          ${HelloNested.getFragment('echoz', {message: variables.message})}
      }
    `,
  }
});

Note: this is how you do it to construct initial query for route. But for subsequent queries, you use setVariables() + props.

[netgusto]

Thank you @vladar for your detailed answer. Applying your advices, I can't get it to work on the React Playground.

It's bugging me how difficult it is to use relay for some simple use cases. Maybe my approach is wrong.

What I've found difficult so far :

• Static query resolution; I understand why it's done that way, and do not complain; but some use cases (using a piece of component state as a variable) are simply not possible (I'd be happy if someone could prove me wrong on the Relay playground above :) )
• Viewer to work around issue Support Root Fields w/o Node Mapping #112
• naming is very slippery; especially, everything seems to breaks if a fragment is named the same as one of it's nested fragments; every parent component has to know the name of the fragments its children expect to be fetched
• the flow of data as props between parent / child is unclear to me; also, very difficult to debug when data does not flow correctly (until the Relay dev tools are released maybe ?)
• No integrated paginated connection; some apps still make good use of paged lists, and do not use infinite scroll

This is not a complain, do not get me wrong. I appreciate the effort the community and FB puts in this project.

Thanks !

[vladar]

Hey, try https://goo.gl/08PZ7g - working version on playground.

Also note that I am not a member of Relay team, so no comments on your points, although I do share some of them.

[josephsavona]

@vladar thanks for answering so many questions here and for the working playground. there's one hand-coded message left though...

@netgusto here is an edited version of the playground with variables passing though from the route through to the bottom component.

Thanks for the great questions and observations. I'll try to provide a bit more context:

Static query resolution; I understand why it's done that way, and do not complain; but some use cases (using a piece of component state as a variable) are simply not possible (I'd be happy if someone could prove me wrong on the Relay playground above :) )

We agree that passing variables in both the static query and as props is not ideal, but it's currently necessary to distinguish between the same container being referenced multiple times with different variables - see my earlier comment. Also note that it's absolutely possible to use component state as a variable - if you're willing to make multiple round trips for data. Relay optimizes for a single round-trip by default using static variables, but for complex use-cases you can use setVariables after the component mounts to fetch additional data (based on the previously fetched data or local state).

naming is very slippery; especially, everything seems to breaks if a fragment is named the same as one of it's nested fragments; every parent component has to know the name of the fragments its children expect to be fetched
the flow of data as props between parent / child is unclear to me; also, very difficult to debug when data does not flow correctly (until the Relay dev tools are released maybe ?)

Fragments can be named the same as children fragments without issue, but I can see how it might have appeared that this wasn't the case while you were debugging other issues. We agree with your second point here that Relay could (should) be more debuggable. Now that the core functionality is stable, this is something we'd like to work with the community on. Ideas and contributions welcome!

No integrated paginated connection; some apps still make good use of paged lists, and do not use infinite scroll

Agreed: we've focused on making infinite scroll easy (since we use it heavily internally), while other forms of pagination are less obvious to implement. Again, this is an area where community contributions would be especially welcome.

[jeromecovington]

I'm pretty well on track with how to do this via @josephsavona's example. But while working with similar code, I am getting the following error:

Error: You supplied a GraphQL document named `InfiniteScroll` that uses template substitution for an argument value, but variable substitution has not been enabled.

This is the fragment I am working with:

fragments: {
    root: (variables) => Relay.QL`
    fragment on Root {
      allContent(first: $first, termSlug: ${variables.termSlug}, taxSlug: ${variables.taxSlug}) {
        edges {
          node {
            id,
            ${FeedCard.getFragment('content')}
          }
        }
        pageInfo {
          endCursor
        }
      }
    `
  }

[josephsavona]

@jeromecovington To reference a variable, just use $var:

initialVariables: {
  first: ...,
  termSlug: ...,
  taxSlug: ..., // <-- be sure to define a default for each variable (or set them to `null`)
},
fragments: {
    root: (variables) => Relay.QL`
    fragment on Root {
      allContent(first: $first, termSlug: $termSlug, taxSlug: $taxSlug) {
        edges {
          node {
            id,
            ${FeedCard.getFragment('content')}
          }
        }
        pageInfo {
          endCursor
        }
      }
    `
  }

[jeromecovington]

@josephsavona - But here I am trying to set the values of termSlug and taxSlug to those passed in via the route. Ex.

class InfiniteRoute extends Relay.Route {
  static routeName = 'InfiniteRoute';
  static paramDefinitions = {
    termSlug: {required: true},
    taxSlug: {required: true}
  };
  static queries = {
    root: (Component, variables) => Relay.QL`
      query {
        root {
          ${Component.getFragment('root', {
            termSlug: variables.termSlug,
            taxSlug: variables.taxSlug
          })}
        }
      }
    `
  };
}

[jeromecovington]

Ah! I see that works...I thought I needed to explicitly reference variables in the query template string. Thanks!

[guzart]

@josephsavona Is there a place to track if the fragment alias will be added to the language? Thank you.

fragment on User {
  foo: ${Child.getFragment('foo')}
}

[josephsavona]

@guzart see graphql/graphql-spec#137

This is something that makes sense for Relay because of the way we do fragment composition with effectively local variables. It isn't so clear if this makes sense for GraphQL clients generally, though, so we may pursue alternatives (for example, a relay directive for providing an alias).

[mickeyinfoshan]

why not use something else instead of props to set variables?

[steveluscher]

Thanks for all of the interesting discussion, everyone. This GitHub issue is going a bit off the rails though; I'm going to close it now that we understand how and when variables get overwritten with props. If anyone would like to raise a follow-on issue, please feel free to open a new one!