Find Breaking Changes: removed directives #1146
Conversation
kenleezle
changed the title
src/utilities/findBreakingChanges.js
Outdated
| const removedDirectives = []; | ||
|
|
||
| oldSchema.getDirectives().forEach(directive => { | ||
| if (!newSchema.getDirective(directive.name)) { |
There was a problem hiding this comment.
This I believe makes this O(n^2): https://github.com/graphql/graphql-js/blob/master/src/type/schema.js#L206
A more efficient version, O(nlogn):
const newSchemaDirectiveNames = new Set(oldSchema.getDirectives().map(directive => directive.name));
oldSchema.getDirectives().forEach(directive => {
if (!newSchemaDirectiveNames.has(directive.name) {
...
}
}
There was a problem hiding this comment.
👍 thanks @mjmahone !
|
Nice, this looks great! Thanks @kenleezle. Can you go ahead and update this PR to also include:
|
kenleezle
changed the title
|
@leebyron CI is not running on my commits, can you ptal? |
src/type/directives.js
Outdated
| @@ -74,6 +75,12 @@ export class GraphQLDirective { | |||
| }); | |||
| } | |||
| } | |||
|
|
|||
| getArgumentMap(): ObjMap<GraphQLArgument> { | |||
There was a problem hiding this comment.
Not sure we want to add this to the Directive class. It should be fine for findBreakingChanges to just use the this.args, and just use Array.find when checking for the existence of an arg. I'm not too worried about performance given that there aren't usually many directives to begin with.
There was a problem hiding this comment.
that was my initial instinct. @mjmahone is that ok with you since you originally raised the concern?
lol @sam-swarr you are going to love what i did with Schema::getDirectiveMap() har har
kenleezle
changed the title
|
@sam-swarr @mjmahone I think I addressed your comments to date. PTAL. Thanks! |
|
closed this and replaced it with #1152 in an attempt to debug the CI not running problem. |
Introduced a function to find removed directives, along with a simple unit test.