Update the relay specific specifications to consider them generic best practices and not purely a relay spec #2603
Conversation
* Minimize the 'These specs are only for Relay' wording for the connection/mutation/objectidentification spec
| mutation fields in a standardized way. These mutations accept and emit a | ||
| identifier string, which allows Relay to track mutations and responses. | ||
| identifier string, which allows Relay Classic to track mutations and responses. |
There was a problem hiding this comment.
While this was a stated intent for Modern, the identifier string (clientMutationId) hasn't actually been removed yet. Not that I disagree with the removal in the spec, but just noting that there will be a mismatch that should be resolved.
There was a problem hiding this comment.
Is it used though? If the functionality still existing in the codebase but isn't a part of how you have to do mutations then it should be fine
There was a problem hiding this comment.
There's nothing in Relay Modern that expects clientMutationId to be there. It's just one possible workaround to fix the issue described here: #1734 (comment)
There was a problem hiding this comment.
Ahh ok, I misremembered. Cool, yeah then it can stay as-is 👍
|
@orta, just to clarify, are you saying that connections and object identifier are becoming part of the graphql spec? Is that documented somewhere? or is it just officially deemed a "best practice" by graphql? cc @josephsavona |
|
@jstejada The latter. This PR is the first step towards detaching these specs from “Relay”. This original comment might provide some more context graphql/graphql-spec#131 (comment) |
|
@alloy that is closed though - is there any actual GraphQL docs that mention these as best practices? |
|
No, there aren't any docs that specify that - as of right now, these Specs are totally considered relay specific. The term best practices could be switched with "GraphQL Schema Extensions" or something among those lines. However, that best practices was the term we used in the meeting a year ago, but can definitely change. The idea being that they are not a part of the spec, but represent a consolidated approach to problems that can/should be adopted in most schemas. |
|
Yeah, I agree that these should be considered best practices, but it would be great if we could have similar messaging on GraphQL docs themselves. Hmm. |
|
This looks great to me as a first step. I'd actually suggest the terminology "Common Patterns" instead of "Best Practices" since what we're trying to do with docs like these is capture common problems people encounter when building GraphQL APIs and the best working patterns to apply to those problems. This isn't really an extension of the schema or spec since these all fit well within the existing spec, they aren't suggesting any additional extensions beyond what GraphQL defines. GraphQL spec just defines the mechanisms by which it works, and the spec itself isn't a great place for trying to codify API patterns like this. However I think the official graphql website docs are a great place for that As a next step, I would suggest opening a PR on the graphql.org website (https://github.com/graphql/graphql.github.io) which updates https://graphql.org/learn/ to contain a new third header on the side bar "Common Patterns" with a document for each of these api patterns. The graphql site also uses markdown for docs, so hopefully porting them over is pretty easy to do. Then, I think it would be helpful to replace the docs on the relay site with a much shorter statement that, if you use these common patterns, Relay leverages them to enable more sophisticated behavior like automatic pagination and smarter caching |
|
Perfect, thanks! I'll get on with this, agree on terminology |
|
I disagree that "common patterns" is appropriate terminology for pagination: "best practice" is the standard terminology to describe a recommended approach to solving a particular problem. The Relay connection spec is the recommended way to implement pagination in GraphQL - per graphql.org, which already includes an informal description of it in the best practices section. We should update that section to include (or link to) the more formal version of the connection spec, and then link to it from the Relay docs as a best practice.
Object identity is a different case - it's a fundamental part of the problem domain that should be covered by the GraphQL spec; the intentional exclusion of this from the spec has led to serious fragmentation in the ecosystem. Relay/Facebook's So to clarify, the steps are:
@orta Interested in updating this PR along those lines and sending PRs to graphql.org? |
|
Yep this makes sense @josephsavona 👍 - if I get time tomorrow I'll move this in that direction |
I don’t feel strongly about this either way, so I’ll leave it to your judgement @orta since you’re graciously doing the work. My only recommendation is that the connection pattern and the node/id pattern are near each other, rather than in separate sections, since we typically recommend using them both with equal vigor and we should help make sure readers that find one easily find the other (or others as we expand docs)
I believe we’ve worked through this a couple times and reached an unfulfilling dead end each time, but I’m happy to work through future proposals since I stilll agree it’s a worthy goal |
|
Hey folks, either this week or next (this week is pretty busy for me) - I'm going to update all these PRs including graphql/graphql.github.io#665 and graphql/graphql.github.io#666 Then for the graphQL website side, I'm going to give a week for them to settle then merge the PRs so that from that side the relay specs are recommended as best practices |
|
OK, I think this is everything - all of the GraphQL website side is ready and in production. I've updated this PR and included links to the graphql pages. Identification lives in: https://graphql.org/learn/global-object-identification/ The Connection spec still lives on Relay, but it is now linked to from the graphQL site: https://graphql.org/learn/pagination/#connection-specification |
There was a problem hiding this comment.
@jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…t practices and not purely a relay spec (#2603) Summary: Better late than never, roughly a year ago in the GraphQL Working Group 3 we took it on Artsy to start moving the Relay specs to be considered "GraphQL Best Practices" ([link](https://github.com/graphql/graphql-wg/blob/master/notes/2018-02-01.md#discuss-moving-connectionglobal-id-specs-from-relay-into-best-practices-repo)) This starts that, beginning with first minimizing the use of Relay as being the sole client that conforms to the connection and object identifier spec. Main notes: - When referring to relay on the client I switched "Relay" to "Spec-compliant client" or "client" - When referring to relay's compatibility on the server side, I oriented it to just be "spec-compliant" - I mostly left the Mutation input spec alone, now that it's only for Relay classic and is close to being removed completely from Relay #2349 - This is step one, once this is merged then I'd like to move both object identification and connection to be on the graphql repos instead of relay and I can handle link updating etc Happy to change wording etc /cc leebyron in case you don't watch this repo too closely. Pull Request resolved: #2603 Reviewed By: josephsavona Differential Revision: D19071019 Pulled By: jstejada fbshipit-source-id: cf5a7dfb93cfd565d4f178a51cdff7cd0caec1a0
Better late than never, roughly a year ago in the GraphQL Working Group 3 we took it on Artsy to start moving the Relay specs to be considered "GraphQL Best Practices" (link)
This starts that, beginning with first minimizing the use of Relay as being the sole client that conforms to the connection and object identifier spec.
Main notes:
clientMutationIdrequirement by creating a new root id for each executed mutation #2349Happy to change wording etc
/cc @leebyron in case you don't watch this repo too closely.