feat: Add directive layer#807
Conversation
michael-georgiadis
changed the title
|
Hey @oojacoboo, I don't want this to become stale. Is it something that you need to get it over the finish line? |
|
Well, for starters, the tests are all going to have to pass. Beyond that, an hour of my time to review it. |
see #811 |
|
@michael-georgiadis #811 was merged. Please rebase and hopefully that resolves tests. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #807 +/- ##
============================================
- Coverage 95.72% 91.71% -4.02%
- Complexity 1773 2022 +249
============================================
Files 154 198 +44
Lines 4586 5429 +843
============================================
+ Hits 4390 4979 +589
- Misses 196 450 +254 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@oojacoboo Done ✅ ! I think the most meaningful cases are covered and I also included an end-to-end one. However, if you want more coverage on specific sub-units happy to add more 🫡 |
|
@oojacoboo Any updates on the review? 👀 |
oojacoboo
left a comment
oojacoboo
left a comment
There was a problem hiding this comment.
@michael-georgiadis this looks really good - thanks! The tests need some additional work though. Firstly, there are custom directives that are still lingering here. Also, there aren't any E2E tests and that's a requirement to get this merged.
After you get that resolved, I think we can get this merged.
| $typeResolver, | ||
| $topRootTypeMapper, | ||
| $this->schemaConfig, | ||
| $directiveRegistry->webonyxDirectives(), |
There was a problem hiding this comment.
I assume this will be updated when we support custom directives?
There was a problem hiding this comment.
Exactly. Then we would have to fetch all the built-ins and also the custom directives specified by the user
# Conflicts: # src/TypeGenerator.php
|
@oojacoboo There was already an E2E test ( Other than that, I also removed the tests that were custom directive related |
There was a problem hiding this comment.
@michael-georgiadis thanks. I still need E2E GQL tests that validate the response.
|
Hey @oojacoboo, I added some that validate the response. Tell me what you think |
|
@oojacoboo any updates on this? |
|
@michael-georgiadis we need E2E GQL tests for all directives, built-in and custom, covering all the various scenarios of use. Deprecation looks fine, but we need a couple OneOf tests. |
|
@oojacoboo added the oneOf tests. Also did a fix that the e2e surfaced. There are no custom directives yet. That will come in a later PR |
|
@michael-georgiadis thanks. Would you mind resolving those other errors so tests pass? I realize it's outside the scope of this PR. If you're unsure, I can get another PR in place to resolve those soon. |
|
@oojacoboo identified the issue and is now fixed. It was indeed unrelated to the changes. Dropped the named argument in favor of positional approach on the If you want to make it more explicit, we can use the |
4 participants
Description
As per your comment on this PR, I am splitting the implementation into two.
I am focusing on configuring the directive layer on this one. Specifying two directives to start with.
How it works
A directive is a PHP attribute implementing one of the family interfaces, which ties it to a GraphQL location (
FieldDirective→FIELD_DEFINITION,InputObjectTypeDirective→INPUT_OBJECT, etc.). Implementing the marker interface alone is metadata-only; implementing the matchingBehavioral*sub-interface adds an apply hook that runs through a middleware pipe during type generation.The built-ins on the layer
#[OneOf]→ sets webonyx'sisOneOfflag → printsinput LookupInput @oneOf.#[Deprecated(reason:)]→ sets the field's deprecation reason → printslegacy: String! @deprecated(reason: "...").Both bind to directives webonyx already declares, so they print through webonyx's own SDL output and we don't register duplicate definitions for them (
DirectiveDefinition::$builtIn).@deprecated: additive, not a rewriteThe existing docblock
@deprecatedsupport is untouched.#[Deprecated]is an attribute-basedpath that composes with it rather than fighting it:
#[Deprecated(reason: 'X')](± docblock)@deprecated(reason: "X")— explicit wins#[Deprecated]+/** @deprecated Y */@deprecated(reason: "Y")— docblock kept#[Deprecated], no docblock@deprecated(reason: "No longer supported")— defaultUsage validation
PHP's
#[Attribute]targets can't tell a#[Type]class from an#[Input]class (both areTARGET_CLASS), so a misplaced#[OneOf]on a#[Type]would otherwise be silently dropped by the interface-based collection.DirectiveValidatorcatches that and throws a clearInvalidDirectiveExceptioninstead.