Maintain allowed legacy names when extending a schema. #1226
Conversation
| } | ||
| `); | ||
| const schema = extendSchema(testSchema, ast); | ||
| expect(schema.__allowedLegacyNames).to.eql(['__badName']); |
There was a problem hiding this comment.
All other tests are usingto.equal so I think it should be consistent.
There was a problem hiding this comment.
I had tried that, but it looks like chai’s equal does an identity comparison, whereas eql does equality comparison. Seeing as the __allowedLegacyNames array gets cloned before passing to the new schema instance, an identity comparison can’t be used here.
There was a problem hiding this comment.
Did you tried to.deep.equal?
There was a problem hiding this comment.
Ah no, I tried deepEqual assuming that Chai was using Node’s assert API. Thanks, will try that 👍
| @@ -90,6 +90,7 @@ const testSchema = new GraphQLSchema({ | |||
| }), | |||
| }), | |||
| types: [FooType, BarType], | |||
| allowedLegacyNames: ['__badName'], | |||
There was a problem hiding this comment.
I don't think it worth to add this option to common test setup
It's better to define separate minimal schema inside test-case itself.
There was a problem hiding this comment.
I like that, I just figured that this file was adding all fixture data to this single schema and followed suit. Happy to revisit and create a tiny schema in the test, though 👍
src/utilities/extendSchema.js
Outdated
| @@ -230,6 +230,8 @@ export function extendSchema( | |||
| types, | |||
| directives: getMergedDirectives(), | |||
| astNode: schema.astNode, | |||
| allowedLegacyNames: | |||
| schema.__allowedLegacyNames && schema.__allowedLegacyNames.slice(0), | |||
There was a problem hiding this comment.
You don't need to use slice since it's defined as:
Line 86 in b333b30
So allowedLegacyNames: schema.__allowedLegacyNames should be enough.
There was a problem hiding this comment.
This is why I needed the equality check, basically. Flow didn’t want to accept a read-only array for the config, which is typed to accept a mutable array:
Line 248 in b333b30
I was surprised too, though, so not being very familiar with Flow I’ll do another sanity check.
There was a problem hiding this comment.
In this case, slice makes perfect sense.
bikeshedding: I like slice() without 0 + it's already used in a couple of places:
graphql-js/src/language/visitor.js
Line 253 in 8bd9fda
|
Thanks for catching this! |
|
@IvanGoncharov Thanks for the review! @leebyron Thanks for making those changes! When this is released I’ll make a PR for Relay to use this and expose a CLI option to specify legacy names. |
v0.13.0-rc.1 adds the
allowedLegacyNamesoption from #1194. Unfortunately, while trying to make a PR for Relay that adds support for this, it turns out that Relay internally usesextendSchemafromgraphql-jsbut that function does not maintain this option and thus the new schema still complains.This PR addresses that, it would be nice if this could go into a v0.13.0-rc.2 release.