-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add ReusableRelay #9
Conversation
@shadowhand This is the next iteration of the "reusable" PR separation. |
namespace Relay; | ||
|
||
class RelayFactory | ||
{ |
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.
I think it would be good to have @var
declarations for the $queue
and $resolver
.
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.
Fixed.
Note that this is an interim PR, intended to split the earlier bigger PR into digestible chunks. As long as the functionality is clear, renaming and docblocks can be part of the final renaming PR. |
|| $queue instanceof ArrayObject; | ||
|
||
if ($getArrayCopy) { | ||
return $queue->getArrayCopy(); |
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.
Would be more useful to use iterator_to_array
instead of array copying? The code could look like this:
protected function getArray($queue)
{
if (is_array($queue)) {
return $queue;
}
return iterator_to_array($queue);
}
This way you can bypass the instanceof
checks entirely and rely on builtins.
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.
Ah so! I like that idea, though it does not apply specifically to the reusability of the relay; that logic pre-exists in the 1.x branch in RelayBuilder. Let's take a look at that separately from this PR.
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.
👍
@shadowhand I think that last push addresses your notes here. Further thoughts? |
Looks good to me. |
Sweet. |
This is an interim PR to add the ReusableRelay functionality without disturbing the previous single-use Relay functionality.
The steps after this would be to:
The renames have the effect of making a Relay reusable by default, and the Runner the single-use dispatcher that the Relay uses.