allow better subclassing of Connection#133
allow better subclassing of Connection#133brennemankyle wants to merge 2 commits intoslackhq:mainfrom
Conversation
…stants in the subclass
| * | ||
| * This should be the Hack class over which you want to paginate. | ||
| */ | ||
| <<__Enforceable>> |
There was a problem hiding this comment.
Why does this need to be enforceable?
There was a problem hiding this comment.
if it's not enforceable I cannot do this in SqlConnection class $node as this::TNode
https://slack-github.com/slack/webapp/pull/128988/files#diff-eeef89f255be1dec860c0b1f779fe2eba85adfebf64bfe7e7c70ab8fef4a8703R47
There was a problem hiding this comment.
I still don’t follow. Looking at that code you shared, $node as this::TNode will explode whenever this::TNode != this::TSqlNode and is a no-op whenever this::Node == this::TSqlNode. No?
| $rc = new \ReflectionClass($class->getName()); | ||
| if ( | ||
| \is_subclass_of($class->getName(), \Slack\GraphQL\Pagination\Connection::class) && | ||
| $rc->getAttributeClass(\Slack\GraphQL\ObjectType::class) |
There was a problem hiding this comment.
Nice.
Should we get a test that this works? E.g., add a FooConnection abstract class subclassing Connection, and a BarConnection concrete class subclassing FooConnection and annotated with ObjectType?
There was a problem hiding this comment.
hmm yeah not really sure where/ how to add this to a test? I added a fixture with sublcassing, but what file do I test it in?
There was a problem hiding this comment.
Just checking that the codegen looks correct should be fine!
There was a problem hiding this comment.
hmm how do I do that? sorry not familiar with out this sort of testing works
There was a problem hiding this comment.
Basically:
- Add the appropriate connection classes to
tests/Fixtures. - Run tests using
make test. This will do the codegen. - Look at the generated files and confirm they look correct. E.g., if you declared an abstract
FooConnectionand a concreteBarConnection, we should end up with one generated class in aBarConnection.hack.
Also happy to pair!
There was a problem hiding this comment.
You also have failing tests... we'll need to annotate UserConnection.hack with the ObjectType attribute, and ideally look at that attribute in Generator::getConnectionObjects. Again, happy to pair.
src/Pagination/Connection.hack
Outdated
| * @see https://relay.dev/graphql/connections.htm for more information about GraphQL pagination. | ||
| * @see src/playground/UserConnection.hack for an example. | ||
| */ | ||
| <<GraphQL\ObjectType('Connection', 'Connection')>> |
There was a problem hiding this comment.
I actually realize this isn't necessary. We don't generate a GQL object for Connection so we don't need to tag the class with this attribute.
|
Kyle Brenneman seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
3 participants
allow subclassing Connection without having to instantiate abstract constants in an abstract subclass
aka Connection > SqlConnection > UserConnection (don't require SqlConnection to have to instantiate TNode)
Also make TNode abstract type enforceable