Validate SDL directives #1435
Closed
Validate SDL directives #1435
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| function validateDirectiveDefinitions(context: SchemaValidationContext): void { | ||
| // Ensure no directive is defined multiple times | ||
| const directiveDefinitions = new Map(); | ||
|
|
There was a problem hiding this comment.
@mjmahone This change is make sense on it's own since you can manually construct GraphQLSchema with duplicated directives.
Can you extract this change into separate PR?
There was a problem hiding this comment.
On Map: all around this library, we use Object.create(null) for maps with strings as keys and Map for non-string keys. AFAIK Map have significantly lover performance, I'm also not sure about memory usage.
|
#1438 is strictly better than this PR, so closing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Re-uses the existing
validateSchemato resolve #1389.This is a sub-optimal solution: ideally we'd be doing validation purely on the AST, as @IvanGoncharov is working on. However, this gets us to a state where people aren't using directives illegally within the SDL, which should allow us to release them.