Skip to content

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