Skip to content
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

Maintain allowed legacy names when extending a schema. #1226

Merged
merged 4 commits into from
Feb 7, 2018

Conversation

alloy
Copy link
Contributor

@alloy alloy commented Feb 7, 2018

v0.13.0-rc.1 adds the allowedLegacyNames option from #1194. Unfortunately, while trying to make a PR for Relay that adds support for this, it turns out that Relay internally uses extendSchema from graphql-js but 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.

}
`);
const schema = extendSchema(testSchema, ast);
expect(schema.__allowedLegacyNames).to.eql(['__badName']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other tests are usingto.equal so I think it should be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you tried to.deep.equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

@@ -230,6 +230,8 @@ export function extendSchema(
types,
directives: getMergedDirectives(),
astNode: schema.astNode,
allowedLegacyNames:
schema.__allowedLegacyNames && schema.__allowedLegacyNames.slice(0),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to use slice since it's defined as:

__allowedLegacyNames: ?$ReadOnlyArray<string>;

So allowedLegacyNames: schema.__allowedLegacyNames should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

allowedLegacyNames?: ?Array<string>,

I was surprised too, though, so not being very familiar with Flow I’ll do another sanity check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, slice makes perfect sense.
bikeshedding: I like slice() without 0 + it's already used in a couple of places:

node = node.slice();

@leebyron leebyron merged commit 4b96a86 into graphql:master Feb 7, 2018
@leebyron
Copy link
Contributor

leebyron commented Feb 7, 2018

Thanks for catching this!

@alloy alloy deleted the extend-schema-and-maintain-options branch February 7, 2018 19:12
@alloy
Copy link
Contributor Author

alloy commented Feb 7, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants