-
Notifications
You must be signed in to change notification settings - Fork 41
Export complexity directive #43
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
Export complexity directive #43
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I added a few suggestions. Could you add some tests and docs for the changes?
src/estimators/directive/index.ts
Outdated
@@ -26,6 +21,10 @@ export default function (options?: {}): ComplexityEstimator { | |||
} | |||
}, | |||
}); | |||
} | |||
|
|||
export default function (options: { name?: string } = {}): ComplexityEstimator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a type for this that we can then also export ComplexityDirectiveOptions
and reuse externally.
src/estimators/directive/index.ts
Outdated
|
||
const directive = new GraphQLDirective({ | ||
name: mergedOptions.name, | ||
export function complexityDirective(name: string = 'complexity') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function complexityDirective(name: string = 'complexity') { | |
export function createComplexityDirective(options?: ComplexityDirectiveOptions = {}) { |
This way it's clearer that the function is not the directive itself but rather a creator, and passing the entire options object keeps the interface stable in case we decide to extend it later.
@ivome I've pushed suggested changes. |
Thanks for adding those changes, looks good now. I'm fine with the workaround for graphql-js <= 14 since it's just in the tests. Once we drop support for the older version, we can remove this, but 14 is still pretty widely used. |
In such a way complexity directive can be reused in code first approach to include directive definition in generated typeDefs: