Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can Route queries only go one level deep? #456

Closed
nickretallack opened this issue Oct 13, 2015 · 12 comments
Closed

Can Route queries only go one level deep? #456

nickretallack opened this issue Oct 13, 2015 · 12 comments
Labels

Comments

@nickretallack
Copy link

I tried to write a more complex query in my route. It didn't work.

Invariant Violation: Relay.QL: Expected query viewer to be empty. For example, use node(id: $id), not node(id: $id) { ... }

Can these queries only go one level deep? Why is this restricted? I already have to give up one level to get out of the one-argument-only restriction or to use connections, which makes these routes pretty useless. The only time I wouldn't want it to be exactly query {viewer} is if I wanted it to be query { node( id: $id ) } instead, because that's the only way I've seen to get Relay to look at its caches.

Anyway, it would be nice if I could put something more complex in the route here. Especially when I'm using react-router-relay. Otherwise I need to create a bunch of wrapper components just to pick up the slack because the queries argument won't do anything for me.

@wincent
Copy link
Contributor

wincent commented Oct 13, 2015

which makes these routes pretty useless.

The essential (and only) function that they serve right now is to anchor a Relay.Container's relatively-specified query fragments at a concrete position in the tree.

The beauty of containers is that they can be composed anywhere, and they don't need to "know" anything about where they will be placed. But we need a route (or "query config" if you prefer) to turn these independent fragments that don't make sense in isolation into something that we can actually send to GraphQL to be executed.

@nickretallack
Copy link
Author

If Relay.Container is so powerful, why do we need Relay.Route?

Instead of writing a container that depends on a route, like:

export default Relay.createContainer(WidgetList, {
  fragments: {
    viewer: () => Relay.QL`
      fragment on Viewer {
        widgets(first: 10) {
          edges {
            node {
              id,
              name,
            },
          },
        },
      }
    `,
  },
});

What if we could write a container that can stand on its own, like this:

export default Relay.createContainer(WidgetList, {
  fragments: {
    query: () => Relay.QL`
      fragment on Query {
        viewer {
          widgets(first: 10) {
            edges {
              node {
                id,
                name,
              },
            },
          },
        }        
      } 
    `,
  },
});

or like this:

export default Relay.createContainer(WidgetList, {
  query: () => Relay.QL`
    query {
      viewer {
        widgets(first: 10) {
          edges {
            node {
              id,
              name,
            },
          },
        },
      }
    }
  `,
});

My first experiences with Relay were very frustrating, mostly because I couldn't figure out how I was supposed to split up my GraphQL query between a Relay.Container and a Relay.Route. Now that I know how limited Relay.Route is, I think it doesn't deserve to exist. It's a piece of boilerplate that only serves to make the API more complicated. Its functionality should be merged into Relay.Container. Then Relay.Container components could stand on their own, which would make examples much simpler.

There is a real lack of testable examples in the documentation right now, partly because you have to understand how to assemble a React.Component in a Relay.Container with a Relay.Route in a Relay.RootContainer before you can do anything. What if all you had to do was put a React.Component in a Relay.Container and be done with it?

@yungsters
Copy link
Contributor

@nickretallack You are absolutely right, and I think there is a good chance we //will// get rid of Relay.Route. Routing will still be important to most applications, but the query configuration portion is kind of unnecessary.

There are a few things currently in progress that are getting us closer to this:

  • We're handicapping the current Relay.Route (as you can see with the existence of RelayQueryConfig). Routing does not need to be strictly tied to Relay in any particular way, as long as you can supply parameters from the router into queries.
  • Relay currently special case fields on the root type in order to reduce redundant fetching of nodes. @steveluscher is working on generalizing this special case so that it can eventually be applied to not only root fields, but arbitrary fields. (Support Root Fields w/o Node Mapping #112)
  • Finally, Relay needs to support fragments on the root type in containers.

@nickretallack
Copy link
Author

Cool. I hope you do make Relay.Container able to stand on its own eventually. What's RelayQueryConfig? Also, what fields does Relay special case? Just nodes? I tried creating a field that was exactly like nodes but was named something else, but it didn't get the same caching behavior.

@yungsters
Copy link
Contributor

RelayQueryConfig is a decomposition of Relay.Route in an effort to remove routing from the Relay vocabulary. It's still a work in progress. :-)

Relay special cases nodes, but it also special cases other root fields. #112 goes into more details. Of particular interest to you is the Rationale for why we are where we are, and the Problem which demonstrates the special cases. If your custom non-nodes query conforms to one of the three outlined types of root fields, you should get similar caching behavior to nodes.

@nickretallack
Copy link
Author

Oh. Maybe I'm mistaken, but when I tried it that did not work. If I fetched a list of things, then tried to fetch the individual thing through nodes it would work without performing another query, while fetching it through some other field it would. Maybe you need to do something special to annotate this field as cacheable?

@josephsavona
Copy link
Contributor

The main issues discussed here are covered by #112 (arbitrary root fiels) and #503 (replacing routes with lighter-weight query configs), so I'm going to close this out.

@dminkovsky
Copy link
Contributor

Ran into this same issue before realizing I should have checked here a long time ago!

Lo and behold.

Thing about this issue is that the documentation encourages you to think that Route can compose its containers fragment deeper than one level down.

For example, on the Routes guide, you get this whole thing about how you can make a ProfilePicture Container and then use compose it with various root queries:

query UserQuery {
  user(id: "123") {
    ...UserProfilePhoto,
  },
}

However, the fragment could also fetch each of user 123's friends' profile photos:

query UserQuery {
  user(id: "123") {
    friends(first: 10) {
      edges {
        node {
          ...UserProfilePhoto,
        },
      },
    },
  },
}

No mention in the Route API reference either.

Given the the docs stress this so hard, I was so fully sold on this that I wasted at least an hour trying to figure out what was wrong with my code!

Not posting on #112 or #503 to avoid polluting those issues, but would be great if Route allowed complex leaves at any depth that composed fragments from its paired container.

@dminkovsky
Copy link
Contributor

I mean, if Routes indeed only allow you get one one level into the graph before deferring to Container fragments, then Containers are not actually reusable because you have to pick up where the one-level-deep Route leaves off. Right?

@josephsavona
Copy link
Contributor

@dminkovsky I'm sorry to hear that this was frustrating. You raise good points about the tradeoff of nesting fields in the query vs container.

Rather than allow more nesting within route queries, we'd like to go in the opposite direction: eliminating the need for routes & their queries, and instead allow express data dependencies solely via containers. @steveluscher is building the prerequisite to this in #112, which adds support for arbitrary root fields.

@dminkovsky
Copy link
Contributor

@josephsavona Oh no, please—thank you for Relay in general and your work here on GitHub. Really appreciate both! Was less expressing frustration and more providing feedback regarding confusion caused by the docs.

If you are planning to eliminate routes and their queries, then all compositing will be done via containers? Got to read #112... :)

Thanks again

@dminkovsky
Copy link
Contributor

I mean, that UserProfilePhoto example from the docs is really cool and it would be nice if it was possible to compose in that manner, one way or another.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants