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

Send operationName for GraphQLOperations #496

Closed
wants to merge 1 commit into from

Conversation

js
Copy link
Contributor

@js js commented Apr 1, 2019

This PR adds operationName to the request payload. operationName is not really required per the graphql spec unless you send multiple operations in the same request. But it is very useful for have when mapping up performance metrics and such so I'd like to have Apollo always send it.

This PR indirectly relies on changes to the apollo codegen, but I'm currently blocked there due to some silly npm tooling issues before I can verify that the apollo codegen:generate part actually works as intended.

Current behaviour (not sending operationName) will be kept until the generator is updated.

The changes suggested in issue #294 only works if you name your operation definition (? what are these actually called) the same as the graphql operation you're calling, eg this is valid graphQL:

query nudgeUser($id: ID!) {
  pingUser(id: $id) { ... }
}

but would give an operationName of "nudgeUser" instead of the actual operation name "pingUser"

@martijnwalraven
Copy link
Contributor

but would give an operationName of "nudgeUser" instead of the actual operation name "pingUser"

Note that this isn't actually correct: the operation name in this case is nudgeUser, that is what operation name means in the GraphQL spec.

pingUser is known as a 'root field', and there can be multiple of these in a single operation. The other side of this is that different operations can include the same root field. So root fields can't be used to uniquely identify operations.

@js
Copy link
Contributor Author

js commented Apr 1, 2019

aha, I missed that part, thank you for correcting me! :)

What threw me off-course is that our graphql server ("apollo-server-core": "1.3.6") rejects the requests unless the operationName matches this root field but that must be something else then…

@js js closed this Apr 1, 2019
@js
Copy link
Contributor Author

js commented Apr 1, 2019

The reason I utterly confused myself when I did as suggested in #294 was because of the inflection the code generator does to operation names, eg: mutation updateFCMToken(..){..} will yield an UpdateFcmTokenMutation swift type, which the GraphQL backend will reject as the operation name UpdateFcmToken!=updateFCMToken.

Dispite my initial confusion/incompetence I think it makes sense to add it as a property which the generator can fill in, rather than relying on the typename which comes with a few gotchas like the above.

@js
Copy link
Contributor Author

js commented May 9, 2019

@martijnwalraven Would love to land this, what do I need to do?

The CI failures for ios9.3 seems to be an unrelated build error:
xctest (5240) encountered an error (Early unexpected exit, operation never finished bootstrapping - no restart will be attempted. (Underlying error: Test runner exited before starting test execution.))

@js
Copy link
Contributor Author

js commented Jul 16, 2019

@designatednerd now that it has been added to the codegen (strangely not from my four month old PR but whatever) could you revisit this so that the client actually sends the operation name in the request? Thanks and sorry for at’ing but you seem like the main driver these days

@designatednerd
Copy link
Contributor

@js Yeah there's a lot of back and forth on that repo - I'm sorry we missed your PR! You can close it if that's the only thing that was in there, or update it and tag me on it if it wasn't.

I am going to be looking at the updated codegen stuff tomorrow (got sucked into a rabbit hole today). Will ping you!

@designatednerd
Copy link
Contributor

@js OK, switching to an updated version of the CLI which automatically generates operation names has now been merged into master - do you want to update this in the next day or so in order to get it shipped with that in 0.13.0?

var operationDefinition: String { get }
var operationIdentifier: String? { get }

var operationName: String? { get }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be nullable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think with the changes in codegen we should be able to make this non-null since it's now auto-generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted for an Optional In case the tooling wasn’t updated (either by the user or within the tooling itself), to soften the dependency

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough - we can certainly leave it optional for a bit but I think eventually we should make this required.

Copy link
Contributor

Choose a reason for hiding this comment

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

heads up: Had to make this non-nullable because protocol resolution stuff made this always return nil even if there was an implemented operationName. I can give a longer explanation if desired but it's basically this example of how protocols with default implementations resolve which implementation to use and why that's not always what you expect.

@designatednerd
Copy link
Contributor

@js Would you have time to get this cleaned up today, or would you prefer I take this over so we can get it shipped with the 0.13.0 changes?

@js
Copy link
Contributor Author

js commented Jul 22, 2019

I’m on vacation so not able to do much for the next week or two @designatednerd feel free to do what’s needed :)

@designatednerd
Copy link
Contributor

Nice! Will do and enjoy your vacation!

@designatednerd
Copy link
Contributor

Closing this one out in favor of #657 - thanks for your patience with all of us @js!

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.

4 participants