-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Evaluate solutions against criteria #650
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this started!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to have a summary table with solutions as rows and evaluation criteria as columns so we can easily see how all of the solutions score against all of the evaluation criteria? The current structure requires lots of jumping around the document to figure out that summary information.
I considered that, but figured that would be problematic if lengthy explanations are required. We might still add that in later, i agree it could be very useful to get an overview. What scale or what kinds of score could we use? I am thinking something simple, like: 😃, 😢 and 😐 ? |
I was thinking something that stands out a bit more, with colors: 🚫 |
Adding in the evaluations in future PRs makes sense, but let's add in one of the negatives of solution 3 before merging so that the community don't mistake it for a clear winner with no drawbacks. For example, we could add: ##### [Doesn't inhibit schema evolution](#c-doesnt-inhibit-schema-evolution)
Adding a nullable field to an input object could change the detected type of
fields or arguments in pre-existing operations. If it needs more explanation, here's an example: Assuming we have this schema:
```graphql
input InputA {
foo: Int
}
input InputB {
foo: Int
bar: Int
}
input InputC {
foo: Int
bar: Int
baz: Int
}
inputUnion InputU = InputA | InputB | InputC
type A {...}
type B {...}
type C {...}
union U = A | B | C
type Query {
field(u: InputU): U
}
```
Given this query:
```graphql
query ($u: InputU = {foo: 3, baz: 3}) {
field(u: $u) {
__typename
... on C {
# ...
}
}
}
```
the default value of `$u` would be detected as type InputC. However adding the
field `baz: Int` to type InputA would result in the same value now being detected
as type InputA, which could be a breaking change for application behaviour. |
@binaryseed i tried the emoji's first, but soon realized that copy-and-pasting them around is stressful, so i went for a more boring:
|
@spawnia ready for me to merge? |
So, I'm thinking the lists of all the criteria repeated over and over for each solution is a bit overwhelming... There's just so much repeated text that I worry the document is getting hard to comprehend... Also, adding any new criteria or solutions will be pretty tedious. Perhaps we should "default" everything to neutral, and only list "Evaluation" items that stand out positive / negative? |
|
||
##### [Input unions should be expressed efficiently in the query and on the wire](#k-input-unions-should-be-expressed-efficiently-in-the-query-and-on-the-wire) | ||
|
||
The additional field bloats the query. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we really considering a single additional field to be "bloat"? The APIs I deal with already have hundreds of fields, I don't think 1 more used in some types makes much difference
This lays out a structure to evaluate each of the solutions against each of the criteria.
I intentionally did not fill out the evaluations yet, as that would blow up the size and need for discussion of this PR. Instead, i propose to do them one-by-one in separate PRs.