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.

Sign up for GitHub

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
Merged

Maintain allowed legacy names when extending a schema. #1226

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5b19c82
Maintain allowed legacy names when extending a schema.
alloy Feb 7, 2018
653083f
.slice() is a more canonical copy
leebyron Feb 7, 2018
dc34d19
Update extendSchema-test.js
leebyron Feb 7, 2018
02577b7
Move schema local to test
leebyron Feb 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/utilities/__tests__/extendSchema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 👍

});

describe('extendSchema', () => {
Expand Down Expand Up @@ -994,6 +995,16 @@ describe('extendSchema', () => {
);
});

it('maintains configuration of the original schema object', () => {
const ast = parse(`
extend type Query {
__badName: String
}
`);
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 👍

});

describe('does not allow extending a non-object type', () => {
it('not an interface', () => {
const ast = parse(`
Expand Down
2 changes: 2 additions & 0 deletions src/utilities/extendSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

graphql-js/src/type/schema.js

Line 86 in b333b30

__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:

graphql-js/src/type/schema.js

Line 248 in b333b30

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:

graphql-js/src/language/visitor.js

Line 253 in 8bd9fda

node = node.slice();

});

// Below are functions used for producing this schema that have closed over
Expand Down