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

Port changes from graphql-js v0.6.0 #136

Closed
wants to merge 26 commits into from
Closed

Conversation

sogko
Copy link
Member

@sogko sogko commented Jun 6, 2016

This version introduces directives to the (still experimental) schema language and introduces a @deprecated directive for marking deprecated fields. It also improves validation error messages

Based of PR #123


Notable changes:

  1. f1164a6 Deepen introspection query from 3 levels to 7
  2. 237fab4 Improve validation error message when field names conflict
  3. 91a3aa2 96fc0ad 96fc0ad Improve validation error messages by listing suggestions
  4. b71c906 Fixes a bug where an empty "block" list could be skipped by the printer.
  5. a19ac6c Export introspection in public API
  6. 9cbbd65 RFC: Schema Language Directives (RFC: Schema Language Directives graphql/graphql-js#376)
  7. 988ab2e RFC: Directive location: schema definition (RFC: Directive location: schema definition graphql/graphql-js#382)
    -This allows directives to be defined on schema definitions.
  8. 1225ab0 Deprecated directive (Deprecated directive graphql/graphql-js#384)
  9. 6fecefe Validation: improving overlapping fields quality (Validation: improving overlapping fields quality graphql/graphql-js#386)

sogko added 12 commits June 1, 2016 14:03
Commit:
6e736bf4733ada6aa15943c0529ed126e8dd8ef1 [6e736bf]
Parents:
86db7340be
Author:
Lee Byron <lee@leebyron.com>
Date:
8 April 2016 at 9:51:04 AM SGT
Labels:
HEAD
Commit:
cbded829525a68b9e0dab61621992cbf01348981 [cbded82]
Parents:
dd02973028
Author:
Zhaojun Zhang <zhaojun@coursera.org>
Date:
26 April 2016 at 2:04:47 AM SGT
Committer:
Lee Byron <lee@leebyron.com>
graphql/graphql-js#364

Commit:
0b72e7038761bec9fb5319cc08ea93ff8b071a9c [0b72e70]
Parents:
cbded82952
Author:
Mike Solomon <mike@mindsnacks.com>
Date:
26 April 2016 at 2:37:25 AM SGT
Committer:
Lee Byron <lee@leebyron.com>
graphql/graphql-js#363

* Improve validation error message when field names conflict

* Remove extra hint and add unit test
…raphql-go#355)

* Add suggestionList to return strings based on how simular they are to the input

* Suggests valid fields in `FieldsOnCorrectType`

* Suggest argument names

* Suggested valid type names

* Fix flow and unit test

* addressed comments in PR: move file, update comment, filter out more options, remove redundant warning

* fix typos

* fix lint

Commit:
5bc1b2541d1b5767de4016e10ae77021f81310fc [5bc1b25]
Parents:
4b08c36e86
Author:
Yuzhi <yuzhi.z@gmail.com>
Date:
27 April 2016 at 2:48:45 AM SGT
Committer:
Lee Byron <lee@leebyron.com>
- `quoteStrings` already defined outside on package scope
Commit:
30a783a2a4f100cad0f31085284895bf51aa565a [30a783a]
Parents:
5bc1b2541d
Author:
Lee Byron <lee@leebyron.com>
Date:
27 April 2016 at 8:49:33 AM SGT
This fixes a bug where an empty "block" list could be skipped by the printer.

Commit:
ea5b241487c884edb561b12e0a92e947107bbfc1 [ea5b241]
Parents:
ec05b5404b
Author:
Lee Byron <lee@leebyron.com>
Date:
5 May 2016 at 7:26:11 AM SGT
Commit Date:
5 May 2016 at 7:26:13 AM SGT
Commit:
88cf354cd65e37fa0793600f6f2a3e6d1b29d21f [88cf354]
Parents:
2ac41f6f19
Author:
Lee Byron <lee@leebyron.com>
Date:
7 May 2016 at 3:09:16 AM SGT
@sogko sogko added this to the v0.6.0 milestone Jun 6, 2016
@sogko sogko changed the title Port changes from graphql-js v0.6.0 [WIP] Port changes from graphql-js v0.6.0 Jun 6, 2016
sogko added 13 commits June 7, 2016 11:41
Commit:
71b6a4aaecf064c7095bca81cc4285fa74ba175e [71b6a4a]
Parents:
980bdf403f
Author:
Lee Byron <lee@leebyron.com>
Date:
7 May 2016 at 4:26:20 AM SGT
This implements adding directives to the experimental schema language by extending the *locations* a directive can be used.

Notice that this provides no semantic meaning to these directives - they are purely a mechanism for annotating an AST - however future directives which contain semantic meaning may be introduced in the future (the first will be `@deprecated`).

Commit:
1b6824bc5df15f8edb259d535aa41a81e2a07234 [1b6824b]
Parents:
71b6a4aaec
Author:
Lee Byron <lee@leebyron.com>
Date:
7 May 2016 at 5:56:25 AM SGT
Labels:
HEAD
This allows directives to be defined on schema definitions.

Commit:
0aa78f61a2dc150b5ea9ee4f50b68a736796f068 [0aa78f6]
Parents:
1b6824bc5d
Author:
Lee Byron <lee@leebyron.com>
Date:
7 May 2016 at 7:05:27 AM SGT
Labels:
HEAD
This adds a new directive as part of the experimental schema language:

```
directive @deprecated(reason: String = "No longer supported") on FIELD_DEFINITION | ENUM_VALUE
```

It also adds support for this directive in the schemaPrinter and buildASTSchema.

Additionally exports a new helper `specifiedDirectives` which is encoured to be used when addressing the collection of all directives defined by the spec. The `@deprecated` directive is optimistically added to this collection. While it's currently experimental, it will become part of the schema definition language RFC.

Commit:
5375c9b20452801b69dba208cac15d32e02ac608 [5375c9b]
Parents:
0aa78f61a2
Author:
Lee Byron <lee@leebyron.com>
Date:
9 May 2016 at 5:56:16 AM SGT
Labels:
HEAD
This adds a new directive as part of the experimental schema language:

```
directive @deprecated(reason: String = "No longer supported") on FIELD_DEFINITION | ENUM_VALUE
```

It also adds support for this directive in the schemaPrinter and buildASTSchema.

Additionally exports a new helper `specifiedDirectives` which is encoured to be used when addressing the collection of all directives defined by the spec. The `@deprecated` directive is optimistically added to this collection. While it's currently experimental, it will become part of the schema definition language RFC.

Commit:
5375c9b20452801b69dba208cac15d32e02ac608 [5375c9b]
Parents:
0aa78f61a2
Author:
Lee Byron <lee@leebyron.com>
Date:
9 May 2016 at 5:56:16 AM SGT
Labels:
HEAD
…ogko/0.6.0

* 'sogko/0.6.0' of https://github.com/sogko/graphql:
  Revert back directives to use `var`; `const` not applicable here
  Deprecated directive (graphql-go#384)
The functions within this validator did not need to close over any state, which allows them to be pure functions.

Commit:
3a01fac4623b37eb7efcdece7a2c33ef74750aeb [3a01fac]
Parents:
5375c9b204
Author:
Lee Byron <lee@leebyron.com>
Date:
10 May 2016 at 6:12:08 AM SGT
Commit Date:
10 May 2016 at 6:12:11 AM SGT
Labels:
HEAD
Two more functions from the overlapping-fields validator which now accept arguments rather than closing over them locally.

Commit:
cf9be874228f4e16762b481d0abed5575f5587db [cf9be87]
Parents:
3a01fac462
Author:
Lee Byron <lee@leebyron.com>
Date:
10 May 2016 at 6:22:53 AM SGT
Commit Date:
10 May 2016 at 6:24:50 AM SGT
…r than fragment AST node

Commit:
688a1ee20db01ca80fdec2189298078380d81ed6 [688a1ee]
Parents:
cf9be87422
Author:
Lee Byron <lee@leebyron.com>
Date:
10 May 2016 at 9:54:10 AM SGT
This improves the overlapping fields validation performance and improves error reporting quality by separating the concepts of checking fields "within" a single collection of fields from checking fields "between" two different collections of fields. This ensures for deeply overlapping fields that nested fields are not checked against each other repeatedly. Extending this concept further, fragment spreads are no longer expanded inline before looking for conflicts, instead the fields within a fragment are compared to the fields with the selection set which contained the referencing fragment spread.

e.g.

```graphql
{
  same: a
  same: b
  ...X
}

fragment X on T {
  same: c
  same: d
}
```

In the above example, the initial query body is checked "within" so `a` is compared to `b`. Also, the fragment `X` is checked "within" so `c` is compared to `d`. Because of the fragment spread, the query body and fragment `X` are checked "between" so that `a` and `b` are each compared to `c` and `d`. In this trivial example, no fewer checks are performed, but in the case where fragments are referenced multiple times, this reduces the overall number of checks (regardless of memoization).

**BREAKING**: This can change the order of fields reported when a conflict arises when fragment spreads are involved. If you are checking the precise output of errors (e.g. for unit tests), you may find existing errors change from `"a" and "c" are different fields` to `"c" and "a" are different fields`.

From a perf point of view, this is fairly minor as the memoization "PairSet" was already keeping these repeated checks from consuming time, however this will reduce the number of memoized hits because of the algorithm improvement.

From an error reporting point of view, this reports nearest-common-ancestor issues when found in a fragment that comes later in the validation process. I've added a test which fails with the existing impl and now passes, as well as changed a comment.

This also fixes an error where validation issues could be missed because of an over-eager memoization. I've also modified the `PairSet` to be aware of both forms of memoization, also represented by a previously failing test.

Commit:
4afb263289485897fdfec37aaf6d5f1e5451dcb3 [4afb263]
Parents:
688a1ee20d
Author:
Lee Byron <lee@leebyron.com>
Date:
11 May 2016 at 5:50:26 AM SGT
Labels:
HEAD
@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 91.473% when pulling 6fecefe on sogko:sogko/0.6.0 into a5cf5f2 on graphql-go:master.

@sogko sogko changed the title [WIP] Port changes from graphql-js v0.6.0 Port changes from graphql-js v0.6.0 Jun 9, 2016
@F21
Copy link

F21 commented Oct 2, 2016

@sogko Any news regarding this one?

@tsunammis
Copy link

@sogko any news ?

* master:
  gofmt -s
  executor: add graphql tag
  Fix ObjectConfig duplicate json tag. Name had a 'description' json tag which was duplicate
  Minor fix to stop `go vet` from complaining
  Add 1.7 and tip to build matrix for Go
  Minor fix to stop `go vet` from complaining
  README: add Documentation section
  Fixes tests that was broken with enhancements made to the `lexer` with commit graphql-go#137
  Improve lexer performance by using a byte array as source
  Updated `graphql.Float` coercion - if value type is explicitly `float32`, return `float32` - if value type is explicitly `float64`, return `float64` - if value type is `string`, return `float64` - for everything else, use system-default
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 83.513% when pulling e13376c on sogko:sogko/0.6.0 into 22050ee on graphql-go:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 83.513% when pulling e13376c on sogko:sogko/0.6.0 into 22050ee on graphql-go:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 83.513% when pulling e13376c on sogko:sogko/0.6.0 into 22050ee on graphql-go:master.

@mishudark
Copy link

Hi, is something blocking this pull request?
there's something that the community can do to finish this PR?

@talk-to-my-car
Copy link

up, why don't you merge this PR ?

@chris-ramon chris-ramon mentioned this pull request Mar 15, 2017
@chris-ramon
Copy link
Member

closing in favor of: #192

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants