Extend relay relationships to support pivot meta on edge

[JasonTheAdams]

Tasks remaining

• Added or updated tests
• Added Docs for all relevant versions
• Updated the changelog

Related Issue/Intent

Resolves #869

Changes
This extends the relate directives — specifically @belongsToMany — to include an edgeType argument. This allows for supplying a custom edge (as opposed to only the generated one). For example:

type User implements Node {
    id: ID! @globalId
    teams: [Team!]! @belongsToMany(type: "connection", edgeType: "TeamEdge")
}

type TeamEdge {
    node: Team,
    cursor: String!
    role: String!
}

Then, when the edge is being resolved the node and cursor fields are handled as they are now, but any additional field is retrieved as pivot data.

This takes advantage of the relay edge type which is intended to reflect the relationship meta between entities. In the world of Laravel this is exactly what pivot data is. Going this route also allows for supporting further control of the edge with directives and such.

Breaking changes

This should not break anything and be fully backwards compatible.

Further Thoughts

I'm considering adding an Edge interface so custom edges can implement it. It could even be that the interface is explicitly required.

[olivernybroe]

I like to solution, I think it makes a ton of sense to do it this way as it also gives the possibility for directives on the pivot fields.

I don't think the interface is a good idea as it adds limitations. I think good exception messages are better here. You could consider adding two directives called something like Node and Cursor. These basically just tells us that this is the field we want to use for cursor and node.
I feel like that would be expressive when reading and give a lot of flexibility regarding naming.

[JasonTheAdams]

@olivernybroe Thanks for chiming in with your vote of confidence!

Respectfully, I disagree with the idea of using directives to identify the node and cursor fields for a few reasons:

1. Lighthouse already handles the resolving of these fields which do end up being a node and string. Allowing the dev to set the cursor as, for example, and int would break things and the dev wouldn't have a way of fixing this.
2. This is within the context of relay, and the current structure is the convention expected for relay. If the dev wants to deviate they shouldn't be using the relay schema structure.
3. That's exactly what interfaces are for: extending an object (type) while remaining confident in the contract's implementation.

If anything, I'd be willing to omit an interface, but I'm not sure why. Lighthouse already has a Node interface, so it seems to make sense to add an Edge one as well to fit this scenario.

[JasonTheAdams]

I figured out where the models were so I was able to added a passing test.

[JasonTheAdams]

One thing I'm debating here is whether the edgeType directive argument is actually necessary. It would be possible to just detect within PaginationManipulator::registerConnection whether the edge type already exists with the given naming convention. Having the argument does allow for it to be explicitly set, however, and have a different type name. 🤔

[spawnia]

Great change overall, i am fine with the design of this.

Can you add a test to cover a missing edgeType?

[spawnia]

One thing I'm debating here is whether the edgeType directive argument is actually necessary. It would be possible to just detect within PaginationManipulator::registerConnection whether the edge type already exists with the given naming convention. Having the argument does allow for it to be explicitly set, however, and have a different type name. 🤔

I wouldn't mind the extra magic, the TypeNameEdge naming seems to be well established.

[JasonTheAdams]

@spawnia Thanks for the feedback and adjustments!

Going the "magic type" method sounds good to me. We'll just have to make sure it's documented. Should I add that to this PR?

That negates the need to check for an invalid type, but I'm still thinking we should make sure it has the expected base structure. What are your thoughts on adding an Edge interface?

interface Edge {
  node: Node
  cursor: String!
}

type TeamEdge implements Edge {
  node: Team
  cursor: String!
  role: String!
}

We could then make sure that the edge type implements that interface. If not, the way it's built now it wouldn't break anything if they simply omitted the cursor or node, but then it's not proper relay schema anymore. I'm inclined to be more strict since this section of schema is determined by Relay, not Lighthouse.

[JasonTheAdams]

Also, with regards to the magic edge type, should we continue to support the edgeType directive argument, or cut it out in favor of the naming convention?

[spawnia]

Also, with regards to the magic edge type, should we continue to support the edgeType directive argument, or cut it out in favor of the naming convention?

Let's allow both. I can see a need for having different edges pointing towards the same type, e.g. UserAuthorEdge and UserSubscriberEdge.

[spawnia]

What are your thoughts on adding an Edge interface?

Let's enforce that. Can you add docs and link to the relevant part of the Relay spec?

[JasonTheAdams]

@spawnia Lots of work done here. Here's what's done:

1. Added support for magic edge type: {$type}Edge
2. Added Edge interface with implementation checking
3. Added checking for whether an item has pivot data, otherwise relies on a custom resolver
4. Added tests for all of this

There is one test that's failing, presently, and that's because in the ASTBuilder::addNodeSupport method checks to see whether the Node interface is ever used and then dynamically loads it. But it's not checking other interfaces. So either we need to expand it to check interfaces or just remove it and add Node support every time. Thoughts?

[spawnia]

Solid tests and implementation, needs only a little polish (and docs)

[JasonTheAdams]

Thanks for the review, @spawnia! Did you happen to see my question regarding loading the Node interface dynamically? #871 (comment)

[JasonTheAdams]

Sorry that I have to keep formatting. I think I've finally tweaked PhpStorm to match what StyleCI is looking for.

[spawnia]

Thanks for the review, @spawnia! Did you happen to see my question regarding loading the Node interface dynamically? #871 (comment)

I don't get what you want to change there, it worked previously?

[spawnia]

Sorry that I have to keep formatting. I think I've finally tweaked PhpStorm to match what StyleCI is looking for.

No worries, i am going to squash the commits anyway.

[JasonTheAdams]

The Tests\Unit\Schema\Directives\PaginateDirectiveTest::itCanChangePaginationAmountArgument method is failing its test. The reason is that the new Edge interface is being added and is referencing the Node interface. The Node interface, however, only gets added if a type is using it. This means it's not loaded because Edge is an interface, not a type.

So we have a couple options:

1. Remove the type checking and load the Node interface every time. The same addNodeSupport method also adds a node query, which I feel like should be determined by config option, not whether or not the Node interface is used, but that may be outside of the scope of this PR.
2. Add interface checking so Node gets loaded if either a type or interface are using it.

Make sense?

[spawnia]

Makes complete sense, thanks for the breakdown.

I agree that we might improve upon the point you mentioned in 1.

For this PR, fixing 2. seems appropriate. Nice catch.

[JasonTheAdams]

Roger that! I went ahead and made an issue to keep track of this. We can discuss a better solution there and make a new PR for that. 👍

[spawnia]

I think we should actually remove the Edge interface again, see https://github.com/facebook/relay/blob/master/website/spec/Connections.md#node

Unfortunately, there is no way in the SDL to require a field named node of the appropriate type. Restricting it to Node is too limiting.

The bug relating to not finding the Node interface should still be fixed, i like how you factored out that method.

[JasonTheAdams]

Ah, I see what you mean, the node field can be more than just an object. That is too limiting. Since it's a required part of the spec, what do you think about doing some validation of our own then? I'm thinking a method that inspects the edge and checks for a node field that is one of the accepted types, and a cursor field that's a non-nullable String.

That said, if the node and cursor fields are omitted the way it's built now it will just not attempt to resolve those fields and act as if there's no problem. So the validation wouldn't be fixing a problem as much as enforcing the Relay spec... hmm. What do you think?

[spawnia]

what do you think about doing some validation of our own then?

... i have not thought of that. Brilliant!

That validation could be run when enabling a config option such as relay.strict, see #878 (comment)

[JasonTheAdams]

@spawnia I removed the Edge interface and usage. Everything is working, now. I'm thinking that by this point this PR has served its purpose. The dev can extend the BelongsToMany directive with their own edge type which supports retrieving pivot data.

I like the direction you're going in #878. Let's make another PR that adds the configuration and validation. I really like the idea of having a "Relay mode" in Lighthouse so I'd be happy to help build that out.

If you're cool with that let's merge this PR and then create the next PR that moves on from this code.

[spawnia]

Nicely done, thanks @JasonTheAdams