Skip to content
This repository was archived by the owner on Nov 12, 2025. It is now read-only.

allow better subclassing of Connection #133

Open
brennemankyle wants to merge 2 commits into
base: main
Choose a base branch
from
Open

allow better subclassing of Connection #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dacfe5e
allow subclassing Connection without having to implement abstract con…
Feb 25, 2022
375a51a
remove annotation
Feb 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Codegen/Generator.hack
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,11 @@ final class Generator {
}

foreach ($classish_objects as $class) {
if (\is_subclass_of($class->getName(), \Slack\GraphQL\Pagination\Connection::class)) {
$rc = new \ReflectionClass($class->getName());
if (
\is_subclass_of($class->getName(), \Slack\GraphQL\Pagination\Connection::class) &&
$rc->getAttributeClass(\Slack\GraphQL\ObjectType::class)
Copy link
Collaborator

@ianhoffman ianhoffman Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@brennemankyle brennemankyle Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@ianhoffman ianhoffman Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking that the codegen looks correct should be fine!

Copy link
Collaborator Author

@brennemankyle brennemankyle Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm how do I do that? sorry not familiar with out this sort of testing works

Copy link
Collaborator

@ianhoffman ianhoffman Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 FooConnection and a concrete BarConnection, we should end up with one generated class in a BarConnection.hack.

Also happy to pair!

Copy link
Collaborator

@ianhoffman ianhoffman Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
invariant(
Str\ends_with($class->getName(), 'Connection'),
"All connection types must have names ending with `Connection`. `%s` does not.",
Expand Down
1 change: 1 addition & 0 deletions src/Pagination/Connection.hack
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ abstract class Connection {
*
* This should be the Hack class over which you want to paginate.
*/
<<__Enforceable>>
Copy link
Collaborator

@ianhoffman ianhoffman Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to be enforceable?

Copy link
Collaborator Author

@brennemankyle brennemankyle Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@ianhoffman ianhoffman Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

abstract const type TNode;

/**
Expand Down