User-defined directives / decorators#123
Conversation
| } | ||
| } | ||
|
|
||
| return $directives; |
There was a problem hiding this comment.
It's easy to miss, but this is what the generated code will look like when we add it to each field: https://github.com/mwildehahn/hack-graphql/pull/123/files#diff-bc6416c0810e5e09cdcf817fbe7fc23de76a1884523575597eb7a6b4bc6b9861R41-R51
There was a problem hiding this comment.
I recommend always generating some of the code as part of the PR so that people can see and review the generated code as if it were human-written. Generally speaking it's best to hold generated code to the same high standards as human-written code.
There was a problem hiding this comment.
Totally. Do you mean that I should include generated code beyond the code this PR includes currently?
There was a problem hiding this comment.
No, I meant that I could not identify by looking at your link that it was a link to lower in this PR. :)
There was a problem hiding this comment.
Gotcha. I'd recommend reviewing this whole file: https://github.com/mwildehahn/hack-graphql/pull/123/files#diff-bc6416c0810e5e09cdcf817fbe7fc23de76a1884523575597eb7a6b4bc6b9861
It contains generated code for object and field-level directives.
src/Codegen/DirectivesFinder.hack
Outdated
| $use_decls = await self::getUseDeclarations($file_path); | ||
|
|
||
| // For each attribute, see if it matches a user-defined directive | ||
| // This requires building the qualified name, which is not |
There was a problem hiding this comment.
You'll probably want to put the "use string operations to build the fully qualified name" into a core utility library.
There was a problem hiding this comment.
Yeah, good call.
| abstract const type TField as shape( | ||
| 'name' => string, | ||
| 'output_type' => shape('type' => string, ?'needs_await' => bool), | ||
| 'directives' => vec<string>, |
There was a problem hiding this comment.
In the spec directives can take arguments so you might want to future proof this and make it dict<string, mixed>. For sure you also don't want to double-up on the directives. And you'll also want key-based lookup instead of C\contains($vec, $thing) which is O(n) linear search.
There was a problem hiding this comment.
Yup, these are just strings for the purpose of this prototype. Agree storing as a dict is better.
src/Codegen/DirectivesFinder.hack
Outdated
| return null; | ||
| } | ||
| $leaves = $decl->getClausesx()->toVec(); | ||
| if ($decl is \Facebook\HHAST\NamespaceGroupUseDeclaration) { |
There was a problem hiding this comment.
HackAst is really slow and changes rapidly along with the language so a lot of it is dangerous to use. Can you use ReflectionClass::getNamespaceName() for this?
There was a problem hiding this comment.
At FB we ban the use of HackAst for anything that's production-critical, including build systems.
There was a problem hiding this comment.
I don't think ReflectionClass::getNamespaceName() will work, at least as the code is written currently: we don't know which class to reflect on until we build the fully-qualified attribute name, which requires parsing the use declarations.
I'm hoping the Facts extension can provide a better alternative to using HHAST, but at Slack we're not yet on a version of HHVM which supports Facts.
There was a problem hiding this comment.
Agreed using HHAST is a bummer. I can play around with ReflectionClass more, but I have to admit I'm not very optimistic about it working.
If developers manually registered their custom directives, then we could use Reflection{Class, Method}::getAttributeClass, which actually does what we want. So maybe that's the interim approach until we support Facts: when creating a new directive, developers register it with the generator:
await GraphQL\Codegen\Generator::forPath(
__DIR__.'/Fixtures',
shape(
'output_directory' => __DIR__.'/gen',
'namespace' => 'Slack\GraphQL\Test\Generated',
'custom_directives' => shape(
'fields' => vec[Directives\HasRole::class],
'objects' => vec[Directives\HasRole::class],
),
),
);That's starting to feel like the cleanest way of doing this.
There was a problem hiding this comment.
Registering feels totally reasonable
There was a problem hiding this comment.
Ok, did that in the latest commit: 355b141
I like where this is headed now. Will work on object directives and introspection next.
3 participants
This PR is a prototype for how we might support user-defined directives / decorators which can be used to, for example, implement permissions. Specifically:
I still need to implement the following:
Stuff I like:
Stuff I don't like:
var_exportto instantiate the directives with the appropriate args within each GQL type.