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

Directives #448

Open
redbar0n opened this issue Dec 19, 2021 · 21 comments
Open

Directives #448

redbar0n opened this issue Dec 19, 2021 · 21 comments
Labels
enhancement New feature or request

Comments

@redbar0n
Copy link
Contributor

redbar0n commented Dec 19, 2021

Would be awesome if GQty would support passing along and extracting (custom) directives.

Official description:

A directive can be attached to a field or fragment inclusion, and can affect execution of the query in any way the server desires. The core GraphQL specification includes exactly two directives, which must be supported by any spec-compliant GraphQL server implementation:

@include(if: Boolean) Only include this field in the result if the argument is true.
@skip(if: Boolean) Skip this field if the argument is true.

Directives can be useful to get out of situations where you otherwise would need to do string manipulation to add and remove fields in your query. Server implementations may also add experimental features by defining completely new directives.

https://graphql.org/learn/queries/#directives

Official Specification:

https://spec.graphql.org/

Highlights:

  • Directives can be used to describe additional information for types, fields, fragments and operations.

  • Directive order is significant

  • @include is a built-in directive

  • @skip is a built-in directive

Examples of custom directives actually in use today:

List of various directives, with good explanation.

Mostly, it seems like directives are only used on fields, for now. Top level, or indented.

Previous discussion:

This same issue in old gqless repo:

samdenty/gqless#210

There are various suggestions there for how the API could look.

@redbar0n
Copy link
Contributor Author

redbar0n commented Dec 19, 2021

But I found none of the suggestions there compelling. I suggest the following GraphQL query to be represented by the following API instead.

# GraphQL
# Example 4: complex example, multiple directives
query MyCachedQuery($skipMember: Boolean!, )
    @cached(ttl: 120)
    @cascade(fields:["name", "city"]) {
  users {
    id
    name @skip
    city @include
    member @skip(if: $skipMember)
    url
    cdn: url @cdn(from: "urlA", to: "urlB")
  }
}

Can be represented as:


const result = query({
  '@cached': { ttl: 120 },
  '@cascade': { fields: ["name", "city"] }
}).users().map(
  u => ({
    id: u.id(),
    name: u.name('@skip'),
    city: u.city('@include'),
    member: u.member({ '@skip': { if: true } }),
    url: u.url(),
    cdn: u.url({ '@cdn': { from: "urlA", to: "urlB" } })
  })
)


Assuming that the query operation name MyCachedQuery is generated by GQty, for caching, and logging/debugging purposes. Also assuming that GQty will generate an alias in the graphql query (e.g. cdn:) upon the second access to the same field (here url()).

Most important presumption here is that all fields would always have to be functions (which again returns the value).
 So that the fields can take a potential directive as input into their function. Might be a breaking change. Note u.id().

Have included the '@' in the directive names. It will be a few more characters to type, but it will be more intuitive to read. If you've worked with GraphQL you would immediately know that the value represented a directive, simply from seeing the '@' key. It also allows avoiding to have an extra directive: {} wrapper or similar simply to make that clear. It could either be enforced, or merely supported by convention.

@redbar0n
Copy link
Contributor Author

@natew @thelinuxlich @jmsegrev @vicary what do you think?

@thelinuxlich
Copy link

I like the idea but I think a more pressing issue is supporting aliases. Graphql-Zeus supports it, for example

@vicary
Copy link
Member

vicary commented Dec 20, 2021

@redbar0n If you read the discussion in quoted issue samdenty/gqless#210, you may already know my opinion, that forcing every scalar into a function defeats the attractive design of gqty/gqless. This maybe resolvable if we figure out a way to do polymorphism for get and apply in proxies.

@thelinuxlich I am curious on how alias are needed. If it is only for multiple input for the same query, there is a current bug that incorrectly serialize the inputs. They are supposed to be managed internally, automagically.

@thelinuxlich
Copy link

There are legit use cases: graphql/graphql-js#522

@redbar0n
Copy link
Contributor Author

redbar0n commented Dec 20, 2021

@redbar0n If you read the discussion in quoted issue gqless/gqless#210, you may already know my opinion, that forcing every scalar into a function defeats the attractive design of gqty/gqless.

Yeah, I read it. What about either naming all the functions getX() so that the usage is clearer? So that .url would become .getUrl() … Alternatively allow methods like urlFn() for fields that can also be accessed with .url if you want the simple scalar?

@vicary
Copy link
Member

vicary commented Dec 21, 2021

@thelinuxlich This only shares the point that we need to explore the API design front. After some thoughts I think possible solutions are diverging from this issue, a new issue/discussion would allow easier follow-ups.

@vicary
Copy link
Member

vicary commented Dec 21, 2021

@redbar0n I, for one, thinks the attractiveness comes from the similarity of a plain object. Any reserved names should be kept minimum and follows a predictable pattern, dynamic names and possible collisions with GraphQL schemas should be avoided.

The problem is focused on the fact that scalars, especially undefined does not allow proxy trapping.

You may try pushing it one level upwards and takes in a path parameter, but that creates ambiguity. I thought of $directiveFor but you may put whatever your favorite name to get the idea.

query.foo.$directiveFor("bar", "@myDirective", { directive: "arguments" });
// ambiguous
query.$directiveFor("foo.bar", "@myDirective", { directive: "arguments" });

@vkrol
Copy link

vkrol commented Dec 22, 2021

Also it was discussed in #proposals channel in Discord.

@thelinuxlich
Copy link

The latest GraphQL spec allows directives to duplicate on the same scope, none of the proposals here are compatible.

@redbar0n
Copy link
Contributor Author

@thelinuxlich duplicate on the same scope? link?

@thelinuxlich
Copy link

graphql/graphql-spec#429

@PabloSzx
Copy link
Member

Also it was discussed in #proposals channel in Discord.

I just made that channel public if you all prefer discussing this there, which should be better, and also suggest to the other people that already have commented and suggested stuff to read the stuff I wrote there a couple months ago with the challenges and design constraints

@redbar0n
Copy link
Contributor Author

redbar0n commented Dec 22, 2021

The latest GraphQL spec allows directives to duplicate on the same scope, none of the proposals here are compatible.

@thelinuxlich My proposal is compatible. You can turn this:

member: u.member({ '@skip': { if: true } }),

Into this:

member: u.member({ '@skip': { if: true }, @include’: { something: true } }),

@redbar0n
Copy link
Contributor Author

@PabloSzx could you summarise the relevant conclusions from your previous discussions?

Chatrooms are good for rapid back-and-forth to hash out suggestions then and there. But I am generally in favor of using github for such discussions, since github is: 1. indexed by search engines, 2. more async, 3. open to the public (while chatrooms are more exclusive / for insiders), 4. easier to contribute to (doesn't require an extra account etc.) 5. contained, i.e. one doesn't have to wade through endless chat-logs to find the relevant tidbits of information.

@redbar0n
Copy link
Contributor Author

I finally found the invite link to the Discord chat.. https://discord.com/invite/U967mp5qbQ

@redbar0n
Copy link
Contributor Author

Ok, I realise you are using a query object as a proxy object for detecting changes to it.

What about this suggestion? Trying to preserve the object-like API.

// #5 - complex GraphQL example, multiple directives, plus duplicate directives on same scope
query MyCachedQuery($skipMember: Boolean!, $includeMember: Boolean!)
    @cached(ttl: 120)
    @cascade(fields:["name", "city"]) {
  users {
    id
    name @skip
    city @include
    member @skip(if: $skipMember) @include(if: $includeMember)
    url
    cdn: url @cdn(from: "urlA", to: "urlB")
  }
}

Represented in GQty JS as:

const skipMember = false
const includeMember = true
query.directives = {
  '@cached': { ttl: 120 },
  '@cascade': { fields: ["name", "city"] },
  'users.name': [{ '@skip': true }],
  'users.city': [{ '@include': true }],
  'users.member': [{'@skip': skipMember, '@include': includeMember }],
  'users.url': [{ '@cdn': { from : "urlA", to: "urlB" } }],
}
const result = query.users.map(
  u => ({
    id: u.id,
    name: u.name,
    city: u.city,
    member: u.member,
    url: u.url,
    cdn: u.url,
  })
)

If the directives are all defined up front like that, the user could probably even avoid having to do the mapping in the last part, and simply do:

const result = query.users

You might ask: Doesn't the query.directives definition here look a lot like the original GraphQL query, so what have we gained? Well, we've contained it to only the directives (if those are needed), and preserved the simple and intuitive API when it is used. GQty will also still generate the queries at runtime, like it normally does, so there is greater flexibility of usage, compared to using GraphQL queries directly. It's only the fields with directives that will be "locked down" by the initial query.directives declaration.

What do you think @PabloSzx and @vicary ?

PS: @skip and @include are duplicate directives on same scope, and wouldn't necessarily make sense, but are here used for illustration purposes.

@vicary
Copy link
Member

vicary commented Jul 3, 2022

@redbar0n The more I think of this, the more I feel this is a fundamental incompatibility between GraphQL and TypeScript.

But on the other hand I am very inclined to move this forward.

I can see that query.directives being a conservative way to not break the existing API, how about we go all the way and hide the whole definition behind util function? The directives definition is a variable only accessible internally.

Developing from the idea of the core resolved(), I can see something like this:

setDirectives(query, {
  '@cached': { ttl: 120 },
  '@cascade': { fields: ["name", "city"] },
  'users.name': [{ '@skip': true }],
  'users.city': [{ '@include': true }],
  'users.member': [{'@skip': skipMember, '@include': includeMember }],
  'users.url': [{ '@cdn': { from : "urlA", to: "urlB" } }],
});

@redbar0n
Copy link
Contributor Author

redbar0n commented Jul 3, 2022

That’s interesting. It for sure is some differences when it comes to patterns and ease of representability.. but TS being turing-complete, I don’t see why it couldn’t in theory represent/output everything GraphQL does. But it does come down to convenience and ease of use, to be worth it, of course..

What would be the intention of hiding query.directives behind a util function? How would users be likely to misuse it if not? Deref the proxy object, or something?

Would query.setDirectives({…}) be an alternative?

@vicary
Copy link
Member

vicary commented Jul 5, 2022

The idea is that every property from the query proxy is a possible naming collision for the top level Query, wrapping it in a util function allows us to move forward before a perfect solution comes up.

@redbar0n
Copy link
Contributor Author

redbar0n commented Jul 5, 2022

Doesn’t GQty control the top level Query, as well as the query proxy object?

@vicary vicary mentioned this issue Nov 10, 2022
8 tasks
@vicary vicary added the enhancement New feature or request label Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants