Cast Int as String if the jsonValue is of type Int. (Custom Scalars better compatibility) #402
Conversation
|
Hey @martijnwalraven, can you take a look at this PR, we're having this issue as well, and looks like it's pretty common. |
| XCTFail("Unexpected error: \(error)") | ||
| } | ||
| } | ||
| let value = try readFieldValue(field, from: object) as! String |
There was a problem hiding this comment.
I would strongly recommend against force-unwrapping here as instead of failing the test, it crashes the whole test suite. Usually I do something like
guard let foo = bar as? String else {
XCTFail("Wrong type!")
return
}
XCTAssertEqual(foo, "expectedFoo")Same deal on the other three tests
There was a problem hiding this comment.
Totally agree. Force unwrapping here is a non go. Your solution sounds very clean to me. I will update this code too.
|
@cadizjavier are you interested in continuing this PR? |
|
Yes, sorry for the long delay. I provided feedback and context to the comments and will update the PR this week. Thanks for taking over this amazing project !! |
|
Hooray! Thank YOU! |
|
Updated PR with your feedback and some improvements in the testing side. There is actually a kinda confusing test which you commented previously regarding |
|
Yeah - after having spent the last week diving into some codegen and typesystem stuff, we can punt on handling the "Well is this a custom scalar or should it have been a string in the first place?" thing until codegen is in swift. I'll make a follow-up issue for it. |
| let object: JSONObject = ["name": 10] | ||
| let field = GraphQLField("name", type: .scalar(String.self)) | ||
|
|
||
|
|
||
| XCTAssertNoThrow(try readFieldValue(field, from: object)) |
There was a problem hiding this comment.
I think this is redundant since the test throws - if this were to throw here or where you're getting the value, the whole test would fail
| let object: JSONObject = ["name": 10] | ||
| let field = GraphQLField("name", type: .nonNull(.scalar(String.self))) | ||
|
|
||
|
|
||
| XCTAssertNoThrow(try readFieldValue(field, from: object)) |
There was a problem hiding this comment.
Samesies on the redundancy of no throw
| @@ -46,10 +46,26 @@ class ParseQueryResponseTests: XCTestCase { | |||
| ] | |||
| ]) | |||
|
|
|||
| XCTAssertNoThrow(try response.parseResult().await()) | |||
There was a problem hiding this comment.
Samesies on the redundancy
|
I think just fixing the test redundancies and this should be 👍 |
|
Eh, I'll remove the redundant checks myself, I'd like to get this out with |
|
Wohooo, excited that this gets approved. Thanks for the feedback and being supportive. |
The support of Apollo for custom scalars is quite simple.
Referring to this issue all custom scalars are mapped to String.
The recommended option for this cases is try to adjust the GraphQL backend for using the
built-inscalars or just pass the--passthroughCustomScalarsflag at thecodegentool.Passing the flag is a really good option but it is super annoying when the Backend defines dozens of custom types that potentially conflicts with the app types.
The solution is to use the
built-inscalars and just expect that everything custom returns as aStringand do the parsing in the Data layer.The main issue with this is that the Apollo parser breaks when the custom scalar is of type
Intsince it fails to do the casting toString.This happens for example when a custom type called
Timestamprepresents the value as1540993726. Then the Apollo parser treats it asStringbut fails to do the casting since isn't aStringreally, it comes in theJSONasInt.This PR solves this. I adjusted all the tests to pass but they need some check if I did something weird there, specially the
testGetScalarListWithWrongTypeone since I don't know if I used the proper approach there.This can be potentially used for Bools too if necessary.
As a side note this kinda helps to fix the following issues without using the
--passthroughCustomScalarsflag and then parse the data to the proper type in the Model layer or something.