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

[Fix] GET Request are failing due to variablesKey cast that always fails. #624

Conversation

dmandarino
Copy link

#623

GET request are always failing because of a cast for in [String: AnyHashable] GraphQLGETTransformer. So I'm changing it to [AnyHashable : Any] because I've checked that it's the type we need to use to use custom headers too.

Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

Looks like this is a limit on the keys I had in the tests for this. Can you please add a test for non-string keys to GETTransformerTests so we can make sure this doesn't regress in the future?

@designatednerd designatednerd added this to the 0.11.2 milestone Jul 11, 2019
@dmandarino
Copy link
Author

@designatednerd Sure, I'm taking a look on it.

Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

Test looks good, just gotta wait ♾️on CI!

@dmandarino
Copy link
Author

@designatednerd it's still stucked. Is it usual to take that long? The other time it was so fast.

@designatednerd
Copy link
Contributor

Yeah during the day in the US the wait time for builds spikes pretty significantly, unfortunately. 😭

@designatednerd
Copy link
Contributor

@dmandarino looks like there's a couple failures in there where occasionally the order of parameters is not the same. I'll take a longer look tomorrow but I suspect the dictionary you're encoding is being generated in a different order.

I'm annoyed that .sortedKeys for JSONSerialization is iOS 11+, I might have to write something for that to support it on older devices.

@@ -44,7 +44,7 @@ struct GraphQLGETTransformer {
queryItems.append(URLQueryItem(name: self.queryKey, value: query))
components.queryItems = queryItems

guard let variables = self.body.jsonObject[self.variablesKey] as? [String: AnyHashable] else {
guard let variables = self.body.jsonObject[self.variablesKey] as? [AnyHashable : Any] else {
Copy link
Contributor

Choose a reason for hiding this comment

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

So messing around in a playground then looking at the docs for JSONSerialization I think this needs to be [String: Any] - JSON Serialization barfs if the keys aren't strings.

@designatednerd
Copy link
Contributor

Ugh, so I looked into this and getting JSONSerialization to sort consistently without using .sortedKeys is stupidly difficult.

I'm almost certainly going to move codegen stuff to Codable in the medium term, so I didn't want to go too far down the rabbit hole of trying to get this to work with JSONSerialization, but it made the test about 900% more annoying to write.

I've opened #628 with my solution to this - do you mind if we move forward with that one since I've already dealt with the test nonsense? Your work here was absolutely critical to getting that to work, and I really appreciate it - I'll make sure this PR gets linked in the changelog!

@dmandarino
Copy link
Author

dmandarino commented Jul 12, 2019

@designatednerd don't worry, just do it! I just wanted to help this project. We all need this feature, specially for persisted query. And I really need this GET, because right now I'm using a fork of my own so the company I'm working on can use Apollo Client for iOS.

And if you don't mind, and if it helps, I can take a look in the other PR too.

@designatednerd
Copy link
Contributor

I don't mind at all - let me know what you think, and I'll close this one out.

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