Skip to content

[FEATURE] addArguments() & addPrefix() to last item in selectionSet - helpers for mghoneimy/php-graphql-oqm #12

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

Closed
wants to merge 7 commits into from

Conversation

actionm
Copy link
Contributor

@actionm actionm commented Aug 28, 2019

I use the package mghoneimy/php-graphql-oqm to generate php classes for a custom graphql endpoint.

Please check out this issue mghoneimy/php-graphql-oqm#1

The addPrefix() function adds the prefix to the last selection element.
The addArguments() function adds the arguments to the last selection element.

@actionm actionm changed the title addArguments() & addPrefix() to last item in selectionSet - helpers for mghoneimy/php-graphql-oqm [FEATURE] addArguments() & addPrefix() to last item in selectionSet - helpers for mghoneimy/php-graphql-oqm Aug 28, 2019
@mghoneimy
Copy link
Owner

I replied to your issue on mghoneimy/php-graphql-oqm#1 on how I think we should approach this.
I don't like that we're catering a solution to add the alias and arguments list just for the last selected field, it doesn't solve the problem we're facing.
I'm more inclined to implement the solution proposed here: mghoneimy/php-graphql-oqm#1 (comment)

@mghoneimy
Copy link
Owner

@actionm I like what you're doing in the second commit, I just have two comments in that:

  1. I propose you move this to another pull request, as it addresses a different issue. It would also make it much easier and faster to review and merge those changes
  2. I'm afraid that replacing the authorizationHeaders with httpOptions will break backaward compatibility. Can we pass the httpOptions as a third optional parameter to the constructor that would override the behavior of the second (which is authorizationHeaders)? This would allow people to initialize the client with different httpOptions depending on their case, and at the same time, not break existing code people are using in their applications.
    For comment (2), should we do this, I will make sure we totally remove the parameter authorizationHeaders when bumping up a major version to 2.0.
    Thank you!

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.

2 participants