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

RFC: Deprecate operationName in favour of kind #1005

Closed
andyrichardson opened this issue Sep 28, 2020 · 1 comment · Fixed by #1045
Closed

RFC: Deprecate operationName in favour of kind #1005

andyrichardson opened this issue Sep 28, 2020 · 1 comment · Fixed by #1045
Assignees
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release

Comments

@andyrichardson
Copy link
Contributor

andyrichardson commented Sep 28, 2020

Summary

Wording is tricky, and there's currently some confusion between the following two

Screenshot 2020-09-28 at 14 55 32

A - Operation type/kind

A finite set of operation types (query, mutation, subscription)

Currently this is indicated in urql on Operation.operationName. It's widely used in the Client'a and Exchanges' control flows and is confusing since it implies to refer to OperationDefinitionNode.name.value, which is ambiguous.

query GetUser # <- `query` is the kind

B - Operation name

One or more user provided names of an operation.

While this is an optional name on the OperationDefinitionNode and not actively observed or used in urql it is sent sometimes by the fetchExchange and subscriptionExchange for instance. This is commonly what's also referred to as the "Operation Name."

query GetUser # <- `GetUser` is the name

Note: A DocumentNode on urql's Operation may contain multiple OperationDefinitionNodes (i.e. multiple queries) with multiple names but that's not supported by convention and it's encouraged, as in other clients, to only pass one, hence an operation will only have one possible name or none.

Proposed Solution

Existing naming

Deprecate the operationName property and introduce a kind property. The deprecated property would be removed in a subsequent major update but should for the time being issue a warning (as a getter) for the next minor bump in development and at most once.

Ruled out

Option 2

Do option 1 and also rename the Operation interfaces (Operation, OperationContext, OperationDebugMeta to use the term Request/UrqlRequest to prevent confusion with the Operation node type in GraphQL (of which, an Urql Operation can have multiple).

Re-introduce the legacy naming scheme as a deprecated alias and subsequently remove in a future major release.

/* @deprecated use `Request` instead */
export type Operation = Request;
/* @deprecated use `RequestContext` instead */
export type OperationContext = RequestContext;

Note: This would take substantially more work including updates to documentation~

Adding support for operation name

Expose the getOperationName utility on @urql/Core by moving it, to allow users to get the operation name of a GraphQL DocumentNode more easily.

Ruled out

Option 2

Add a operationNames property to the existing Operation type

@andyrichardson andyrichardson added the future 🔮 An enhancement or feature proposal that will be addressed after the next release label Sep 28, 2020
@kitten
Copy link
Member

kitten commented Sep 28, 2020

I think option 2 isn't too important or possible, as the Operation term is very engrained. This would also conflict with our existing usage of the terms "Request" both for HTTP requests and the raw GraphQL Request (key + document + variables without any other metdata)

Overall I'd be happy to keep the Operation terminology as it's been very insightful for users and has worked well to describe the inner "primitive to start a GraphQL operation / IO request+response". This also pairs up nicely with an "operation" in common GraphQL terminology matching up nicely with our idea of an Operation (it matches having variables and the document, where the document defines the kind of operation anyway; just with our metadata added to it)
so I think we can remove "option 2" and go for "option 1" completely.

Adding support for operation name

Totally agreed on "option 1" here as that utility exists and the name is already set and matches up with general "GraphQL AST utility naming", so no new name is needed and this would stop being ambiguous. "option 2" isn't necessary here I think either.

Side note on deprecation of operationName

In the minor change that deprecates operationName, we can add a getter for it that still returns the kind property value of 'query' | 'mutation' | 'subscription' but also issues a one-off warning that the property is deprecated in development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants