Skip to content
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

+InlineFragment fields in ResolveInfo->getFieldSelection() #61

Closed
wants to merge 3 commits into from

Conversation

rudiedirkx
Copy link

Background starting at #60 (comment)

$fragmentFields = $this->foldSelectionSet($selectionAST->selectionSet, $descend - 1);
$fragmentType = $selectionAST->typeCondition->name->value;
foreach ($fragmentFields as $name => $subFields) {
$fields[$fragmentType . ':' . $name] = $subFields;
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace $fragmentType . ':' . $name with just $name? Otherwise it breaks the contract of this function (where you could just query result by field name).

Copy link
Author

Choose a reason for hiding this comment

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

Not really. The type condition is important, because implementor A and B could both query field X, but have different children, because they mean something else:

... on A {
  some {
    type
  }
}
... on B {
  some {
    kind
  }
}

some isn't in the interface, because they're a different kind of some, they just have the same name.

Would B win and overwrite A's type?

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Need to think about it before merging

Copy link
Author

Choose a reason for hiding this comment

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

Maybe a new kind of nesting level instead of prefixing the key? Not very bc. Or returning a new, array accessible object with meta data like this? Could be bc, but might be overkill.

Also, I'm not sure this patch includes all cases: inline fragment, named fragment, inline fragment inside named fragment, etc. I've tried only 2 of many scenarios.

@coveralls
Copy link

coveralls commented Nov 1, 2016

Coverage Status

Coverage decreased (-0.1%) to 88.58% when pulling 6e0904b on rudiedirkx:inlinefragment into 4a75bc6 on webonyx:master.

@rudiedirkx
Copy link
Author

Have any brilliant solutions come up? With this, or something like it, I can make very efficient getters.

@vladar
Copy link
Member

vladar commented Nov 1, 2016

@rudiedirkx Not really. I guess good solution requires breaking change anyway.

I incline to deprecating this function and introducing new one wich will handle Inline fragments in more elegant way.

I guess I could merge this, but don't see much point since it will be deprecated anyway.

But technically it shouldn't block you. Just extract this piece of logic into your own utility function accepting ResolveInfo as argument and do the same.

Maybe eventually you will come up with brilliant solution and contribute it back %)

@rudiedirkx
Copy link
Author

Sure, I could use my own method, but I rather use your shiny new very awesome new method. I'm very curious to see this more elegant way. Sneak preview anywhere? Create issue to track progress? You can close this PR.

@vladar
Copy link
Member

vladar commented Nov 4, 2016

@rudiedirkx See #65 to track this. Looking forward to hear your thoughts and ideas.

@vladar vladar closed this Nov 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants