Port 0.12.3 changes from graphql-js #248
Conversation
This RFC adds a new form of `StringValue`, the multi-line string, similar to that found in Python and Scala. A multi-line string starts and ends with a triple-quote: ``` """This is a triple-quoted string and it can contain multiple lines""" ``` Multi-line strings are useful for typing literal bodies of text where new lines should be interpretted literally. In fact, the only escape sequence used is `\"""` and `\` is otherwise allowed unescaped. This is beneficial when writing documentation within strings which may reference the back-slash often: ``` """ In a multi-line string \n and C:\\ are unescaped. """ ``` The primary value of multi-line strings are to write long-form input directly in query text, in tools like GraphiQL, and as a prerequisite to another pending RFC to allow docstring style documentation in the Schema Definition Language. Ref: graphql/graphql-js#926
ref: graphql/graphql-js#f9e67c403a4667372684ee8c3e82e1f0ba27031b
As discussed in graphql/graphql-spec#90 This proposes replacing leading comment blocks as descriptions in the schema definition language with leading strings (typically block strings). While I think there is some reduced ergonomics of using a string literal instead of a comment to write descriptions (unless perhaps you are accustomed to Python or Clojure), there are some compelling advantages: * Descriptions are first-class in the AST of the schema definition language. * Comments can remain "ignored" characters. * No ambiguity between commented out regions and descriptions. Specific to this reference implementation, since this is a breaking change and comment descriptions in the experimental SDL have fairly wide usage, I've left the comment description implementation intact and allow it to be enabled via an option. This should help with allowing upgrading with minimal impact on existing codebases and aid in automated transforms. BREAKING CHANGE: This does not parse descriptions from comments by default anymore and the value of description in Nodes changed from string to StringValueNode
* Adds support for resolving union/interface types when using a generated schema * Move resolveType __typename checking into defaultResolveType * Clean up existing tests and improve error messages ref: graphql/graphql-js#947 # Conflicts: # src/Utils/BuildSchema.php # tests/Utils/BuildSchemaTest.php
ref: graphql/graphql-js#72421378550cf51b13c6db59b8fc912591fd1a4b
ref: graphql/graphql-js#bf4a25a33a62280e82680518adc279e34ec816e0
ref: graphql/graphql-js#999 # Conflicts: # src/Utils/BuildSchema.php
ref: graphql/graphql-js@db4cfdc ref: graphql/graphql-js#1096 # Conflicts: # tests/Utils/FindBreakingChangesTest.php
* Generalizes building a value from an AST, since "scalar" could be misleading, and supporting variable values within custom scalar literals can be valuable. * Replaces isNullish with isInvalid since `null` is a meaningful value as a result of literal parsing. * Provide reasonable default version of 'parseLiteral' ref: graphql/graphql-js@714ee98 ref: graphql/graphql-js#903 # Conflicts: # src/Utils/BuildSchema.php # tests/Utils/BuildSchemaTest.php
This changes the check for null/undefined to a check for undefined to determine if scalar serialization was successful or not, allowing `null` to be returned from serialize() without indicating error. This is potentially breaking for any existing custom scalar which returned `null` from `serialize()` to indicate failure. To account for this change, it should either throw an error or return `undefined`. ref: graphql/graphql-js#1104
ref: graphql/graphql-js#1000 BREAKING CHANGE: SchemaBuilder::build() and buildAST() and constructor removed the typedecorator, as not needed anymore as library can now resolve union and interfaces from generated schemas.
This changes the parsing grammar and validation rules to more correctly implement the current state of the GraphQL SDL proposal (graphql/graphql-spec#90) ref: graphql/graphl-js#1102
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.
ref: graphql/graphql-js#386
`OverlappingFieldsCanBeMerged` would infinite loop when passed something like
```graphql
fragment A on User {
name
...A
}
```
It's not `OverlappingFieldsCanBeMerged`'s responsibility to detect that validation error, but we still would ideally avoid infinite looping.
This detects that case, and pretends that the infinite spread wasn't there for the purposes of this validation step.
Also, by memoizing and checking for self-references this removes duplicate reports.
ref: graphql/graphql-js#1111
This adds a new function `getIntrospectionQuery()` which allows for some minor configuration over the resulting query text: to exclude descriptions if your use case does not require them. ref: graphql/graphql-js#1113
This adds the recent changes to the SDL proposal. ref: graphql/graphql-js#1117
A common case is encountering an error which blames to a single AST node. Ensure the GraphQLError constructor can handle this case. ref: graphql/graphql-js#1123
ref: graphql/graphql-js@007407d Also fixes expected <-> actual in this testfile
This factors out the enum value validation from scalar value validation and ensures the same try/catch is used in isValidLiteralValue as isValidPHPValue and protecting from errors in valueFromAST. ref: graphql/graphql-js#1126
Lifted from / inspired by a similar change in graphql/graphql-js#722, this creates a new function `printError()` (and uses it as the implementation for `GraphQLError#toString()`) which prints location information in the context of an error. This is moved from the syntax error where it used to be hard-coded, so it may now be used to format validation errors, value coercion errors, or any other error which may be associated with a location. ref: graphql/graphql-js BREAKING CHANGE: The SyntaxError message does not contain the codeframe anymore and only the message, (string) $error will print the codeframe.
This includes: graphql/graphql-js#1147 graphql/graphql-js#355 This also fixes two bugs in the Schema - types that were not found where still added to the typeMap - InputObject args should not be searched for types.
This generalizes the "arguments of correct type" and "default values of correct type" to a single rule "values of correct type" which has been re-written to rely on a traversal rather than the utility function `isValidLiteralValue`. To reduce breaking scope, this does not remove that utility even though it's no longer used directly within the library. Since the default values rule included another validation rule that rule was renamed to a more apt "variable default value allowed". This also includes the original errors from custom scalars in the validation error output, solving the remainder of graphql/graphql-js#821. ref: graphql/graphql-js#1144
ref: graphl/graphql-js#1149
For misspelled enums or field names, these suggestions can be helpful. This also changes the suggestions algorithm to better detect case-sensitivity mistakes, which are common ref: graphql/graphql-js#1153
danez
changed the title
|
Okay I added all the relevant commits from 0.11 till 0.12.3 from graphql-js. Major changes are the new SchemaDefinitionLanguage from the spec, a new SchemaValidator, updated validation rules, more breaking changes detection. Lots of validations stuff was already removed from the constructor here, which they now also did in the JS library. I tried to sync the two libraries in this topic and moved some logic from TypeClasses (for example Directive) to the I also run phpbench. Before and after So the seems most benchmarks didn't change much. Let me know what you think, and sorry again for the huuuuggggeee PR. If I can make your life easier somehow with reviewing let me know. I created the checkboxes in the initial description that you might want to use to tick reviewed commits if you want to do reviews based on commits. If this is done and you are happy with what I'm doing I can continue and move to 0.13, but for now I need a break :D |
|
This is fantastic! I wanted to port these changes in near future, just couldn't find big-enough time window (as I've been quite busy recently). So your PR is sooo helpful, thanks again! But reviewing it will also take some time %) |
|
One major problem is a removal of a As far as I understand regular buildSchema doesn't allow you to do this and just asks to rely on root value for resolution process? If so, then this is something we will probably have to revert. |
|
Okay I understand, I will add it back. |
|
Okay I added it back and also fixed a bug with the lazy loading of types which I introduced. |
|
Merged! One final thing that concerns me is that we expose How do you think can we just throw instead of returning undefined? Looks like exceptions in those methods are handled correctly, so there shouldn't be a major difference, right? |
|
And thank you so much for this huge work! |
|
🎉 Feel free to ping me if bugs come up. |
|
@danez So any ideas about throwing vs returning |
|
Ah sorry I missed that comment. Yes good point, I guess throwing would have the same effect as |
|
I will create a PR to do the |
|
Cool, as soon as we switch to throw vs |
Instead of doing one PR for each and every change I now decided to do everything in one branch because I started getting tons of merge conflicts as the changes usually depend on other changes.
For review each commit could be reviewed separately.
I hope this is okay, I know that huge PRs are a pain to review, but otherwise it would block me completely.
I will list the changes here:
0.12.3
stringtoStringValueNodebuildASTSchema,extendSchemaandbuildClientSchemagraphql/graphql-js#903 | graphql/graphql-js@714ee98Fixes not related to 0.12 or 0.13
Docs