Fix issue with Relay serialization #2638
Conversation
|
For some reason it's not picking up a dependency. |
|
@freiksenet i'll drop a line here when directives sdl support will be added to ETA today or tomorrow |
|
Done |
| ); | ||
| function buildSchema() { | ||
| // Compose upstream is going to add AST directives soon, making this simpler | ||
| const composer = new SchemaComposer(); |
There was a problem hiding this comment.
graphql-js supports extending schemas, can we just use that?
There was a problem hiding this comment.
Could you point me to that? extendSchema only support extending via AST.
| __typename_mutated | ||
| } | ||
| friends_mutated(after_mutated: $after_mutated, first_mutated: $first_mutated, traits_mutated: [HELPFUL_mutated]) { | ||
| friends_mutated(after_mutated: $after_mutated, first_mutated: $first_mutated, traits_mutated: [DERISIVE]) { |
There was a problem hiding this comment.
let's keep the original N/N_mutated style just to make it more obvious what the test is doing
There was a problem hiding this comment.
I can't, serialization for enums only returns valid values and HEPLFUL_mutated isn't a valid enum value for traits.
| type.name, | ||
| value, | ||
| ); | ||
| return value; | ||
| return result; |
There was a problem hiding this comment.
I can't remember if GraphQLInputObject and GraphQLList also support serialize(), if they do can we just use the approach here for all types (ie replace the body of printLiteral w the body of the enum case)? Even if lists/objects don't work, we could do this for all enums and scalars.
There was a problem hiding this comment.
Only scalars and enums have serialize.
| @@ -13,6 +13,7 @@ | |||
| "dependencies": { | |||
| "@babel/runtime": "^7.0.0", | |||
| "fbjs": "^1.0.0", | |||
| "graphql-compose": "^5.8.0", | |||
There was a problem hiding this comment.
Same question, we should use the built-in mechanism to extend schemas.
There was a problem hiding this comment.
It's my "lodash" for GraphQL type system 😈
If not today then it will be used tomorrow. Just give me some time to make this lib popular and right solution for such kind of problems.
There was a problem hiding this comment.
Which mechanism? extendSchema only supports extending through AST.
|
It's pretty critical for us to get this merged and released, because 2.0.0 is breaking for us (Gatsby). If you can't merge additional libraries for test, then I think this should be merged without new tests. |
|
Appreciate y'alls help getting this merged! |
Right - what I'm wondering is if we can start with a very minimal schema defined in code, and then extend it with an AST schema derived from the testschema.graphql file. I'm concerned about adding third-party dependencies to something as critical as our core test suite (which could affect both test performance and reliability). If that doesn't work, let's split this into two steps as you suggested: first we can land the printer changes to use |
Right, I didn't think about doing it in other direction. I'll try to do it tomorrow. |
|
OK, I had to do double extend for this to work, because there seems to be a bug in AST Builder that sometimes makes it not use the defined type map function, probably when it's input types or maybe argument types. |
|
Any update on this? |
|
Disregard the closure, I misclicked when commenting. |
|
Pushed additional test for another regression we found that is fixed by this. |
There was a problem hiding this comment.
@josephsavona has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
I manually updated the imported version to account for the latest changes - note that I had to make some changes for backwards compatibility purposes for internal code, so updates on this PR won't apply cleanly to the imported version. Do you foresee any more changes? edit: I'm landing this, so too late for changes! |
|
Thanks for letting us know about this and sending a fix! |
Fixes #2633.
I had to add dependency to actually add enums with proper test values, because monolithic SDL-first is so limited.