Skip to content

Conversation

m14t
Copy link
Contributor

@m14t m14t commented Dec 6, 2019

Description

This in an implementation for a spec proposal:

Example usage

Source

const { graphql, buildSchema } = require("graphql");

const schema = buildSchema(`
  """
  A UUID
  """
  scalar UUID @specified(by: "https://tools.ietf.org/html/rfc4122")
  
  type Query {
    someUUID: UUID
  }
`);

const root = {
  someUUID: () => {
    return "750fce86-fc2f-4b25-a7df-fd0941d3fa08";
  }
};

const query = `
{
  __type(name: "UUID") {
    kind
    name
    description
    specifiedBy
  }
}`;

graphql(schema, query, root).then(response => {
  console.log(JSON.stringify(response, null, 4));
});

Output

{
    "data": {
        "__type": {
            "kind": "SCALAR",
            "name": "UUID",
            "description": "A UUID",
            "specifiedBy": "https://tools.ietf.org/html/rfc4122"
        }
    }
}

Notes

Please let me know if I forgot something.

@IvanGoncharov IvanGoncharov added spec RFC Implementation of a proposed change to the GraphQL specification PR: feature 🚀 requires increase of "minor" version number labels Dec 7, 2019
@m14t
Copy link
Contributor Author

m14t commented Dec 7, 2019

@IvanGoncharov Thanks for the review!

While trying to improve the test coverage here, I wanted pass an SDL that used @specified to cycleSDL, but ran into #2020.

I'm not entirely sure of the implications of this, but if more work is need and given some direction, I'd be happy to continue to work on this.

cc: @eapache

Copy link
Member

@eapache eapache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak to the question about schema printer but the rest of this LGTM.

Thanks for the help Matt!

@IvanGoncharov
Copy link
Member

While trying to improve the test coverage here, I wanted pass an SDL that used @Specified to cycleSDL, but ran into #2020.

@m14t It should be handled the same way as @deprecate.
#2020 is about printing user-provided directives and there is nothing wrong in hardcoding standard directives for a time being.
Also please review tests that mention @deprecate or deprecatedReason and replicate ones that make sense for @specified/specifiedBy.

@m14t m14t force-pushed the specified-directive branch 2 times, most recently from 8bb758d to e76581b Compare December 16, 2019 03:09
@m14t
Copy link
Contributor Author

m14t commented Dec 16, 2019

@IvanGoncharov thank you for the feedback.

I've made the requested changes, and feel better about this. I still need to check that I have all of the test cases, but think that the implementation is done.

@IvanGoncharov
Copy link
Member

I still need to check that I have all of the test cases, but think that the implementation is done.

@m14t You should have 100% coverage for diff ATM it's 97.56%:
image

@m14t m14t force-pushed the specified-directive branch 4 times, most recently from 74bb0d3 to 90e650e Compare December 31, 2019 02:18
m14t added a commit to m14t/graphql-wg that referenced this pull request Jan 2, 2020
I've been working on the [implementation](graphql/graphql-js#2276) of the `@specified(by: "")` directive as specified by graphql/graphql-spec#649, and am interested an any feedback or progress on that effort.
@IvanGoncharov
Copy link
Member

@m14t Can please update this PR to be in sync with graphql/graphql-spec@b418b60 ?

m14t added a commit to m14t/graphql-wg that referenced this pull request Jan 9, 2020
I've been working on the [implementation](graphql/graphql-js#2276) of the `@specified(by: "")` directive as specified by graphql/graphql-spec#649, and am interested an any feedback or progress on that effort.
@m14t m14t changed the title Add @specified directive Add @specifiedBy directive Feb 5, 2020
@m14t m14t force-pushed the specified-directive branch from 1377aeb to 18689db Compare April 25, 2020 17:14
@m14t
Copy link
Contributor Author

m14t commented Apr 25, 2020

@IvanGoncharov Sorry for the delay here, but this is ready for another review when you have a chance.

@m14t m14t force-pushed the specified-directive branch 2 times, most recently from 4dcf49b to c4a6a0b Compare April 28, 2020 15:29
@m14t m14t force-pushed the specified-directive branch from c4a6a0b to 17d4ad3 Compare April 29, 2020 17:05
@IvanGoncharov
Copy link
Member

@m14t Sorry for the delay will try to review, merge, release before tomorrow's WG.

+kind: 'SCALAR',
+name: string,
+description?: ?string,
+specifiedByUrl: ?string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically a breaking change: code that was using IntrospectionScalarType no longer type-checks unless specifiedByUrl is explicitly set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number spec RFC Implementation of a proposed change to the GraphQL specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants