Refactor the stitching code, update to the latest graphql-tools #1028
Conversation
| @@ -38,3 +38,4 @@ NODE_ENV=test | |||
| PORT=5001 | |||
| QUERY_DEPTH_LIMIT=10 | |||
| RESIZING_SERVICE=gemini | |||
| REQUEST_TIMEOUT_MS=5000 | |||
There was a problem hiding this comment.
we had tests that relied on this
| "apollo-link-http": "^1.1.0", | ||
| "apollo-link": "1.2.1", | ||
| "apollo-link-context": "^1.0.8", | ||
| "apollo-link-http": "^1.5.4", | ||
| "artsy-morgan": "git://github.com/artsy/artsy-morgan.git", |
There was a problem hiding this comment.
The specific apollo version is because of ardatan/graphql-tools#765
| }) | ||
| mergedSchema.__allowedLegacyNames = ["__id"] | ||
| return mergedSchema | ||
| } |
| }, | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Our first original merging regression, moved into its own file.
| @@ -92,53 +91,3 @@ describe("convection link", () => { | |||
| }) | |||
| }) | |||
| }) | |||
There was a problem hiding this comment.
This file tests all the HTTP headers in the link now
| const newName = remap[name] || name | ||
| return newName | ||
| }), | ||
| ]) |
There was a problem hiding this comment.
This is just a simple rename for the convection types to be the same as what exists inside MP today.
| }, | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Basically lets each service keep their stitching to themselves. If/when we get to a point where two are trying to stitch resolvers on to the same object, we can deal with it then
| @@ -100,7 +100,7 @@ export const SubmissionType = new GraphQLObjectType({ | |||
| fields: { | |||
| ...GravityIDFields, | |||
| artist_id: { | |||
| decription: "The gravity ID for an Artist", | |||
| description: "The gravity ID for an Artist", | |||
| type: new GraphQLNonNull(GraphQLString), | |||
There was a problem hiding this comment.
I spotted one and figured there might be more
| import schema from "schema" | ||
| import { mergeSchemas } from "lib/mergeSchemas" | ||
| // Please do not add schema imports here while stitching is an ENV flag | ||
| // |
There was a problem hiding this comment.
The move from static imports to dynamic is so that we can make guarantees about the merged schemas. It won't matter once it's removed from the env var though.
|
|
||
| preprocessors: { | ||
| "**/*.js": wallaby.compilers.babel(JSON.parse(babelRC)), | ||
| "**/*.ts": wallaby.compilers.babel(JSON.parse(babelRC)), | ||
| }, | ||
|
|
There was a problem hiding this comment.
I'm the only person using wallaby, so I just keep it up to date for myself
| // metaphysics ecosystem. | ||
| const remap = { | ||
| Submission: "ConsignmentSubmission", | ||
| Category: "SubmissionCategoryAggregation", |
There was a problem hiding this comment.
Curious why the Aggregation naming? But 👍 love this!
There was a problem hiding this comment.
That was the original name in metaphysics, can't remember why it's called that now, but likely was replicating something on convection
| convectionSchema, | ||
| convectionStitching.extensionSchema, | ||
| ], | ||
| onTypeConflict: (leftType, rightType) => { |
There was a problem hiding this comment.
Curious about this- gravitySchema has Artwork and other conflicting types, so based on this handler now, are we getting errors in development now?
ie- I recall seeing Type Collision Artwork and the like in the existing version. Since we want to keep those from Metaphysics, should we add them to scalarsToKeepInMetaphysics (and rename it to drop 'scalar')?
There was a problem hiding this comment.
I took out merging of gravity's graphql for this as it's not being used, will figure that one out later but we will probably need to either do a full switcheroo with MP's Artist, or rename Grav's GraphQL for the time being.
WRT scalars, those are core primitives that are definitely the same, merging and overwriting we should be handling on a case by case basis
There was a problem hiding this comment.
Wait, but we are still merging in gravity GraphQL? (or am i misunderstanding the term 'merging' and 'stitching', etc. again 😆 )
Also, there is a mutation we added in Gravity's GraphQL that we wanted to use transparently thru Metaphysics (recordArtworkView).
There was a problem hiding this comment.
Ah, I'm surprised that removing it didn't break a test then, sure I can get it merged back in with #1032
There was a problem hiding this comment.
Whoops! I added it in the same style as the Convection ones, there is a mutation in Metaphysics that directly matches the one in Gravity's GraphQL, and which one is used (this was the goal), is behind the ENABLE_SCHEMA_STITCHING flag.
There was a problem hiding this comment.
ah, that makes sense then yeah! Cool, I'll re-create a version of that test that uses stitching 👍
|
Closing in favor of #1032 |
I'll comment inline