Foundational Work For Codegen Rewrite #1817
Conversation
- Setup Package.swift - Renamed and moved files - Fixed imports in Apollo - Refactored call sites - Renamed Reference to CacheReference - Use Dictionary(_: uniquingWithKeys:) instead of custom initializer in extension. TODO: - Fix call sites and imports in ApolloWebSocket and ApolloSQL - Cocoapods & Carthage - Fix call sites in tests
TODO: Fix unneeded import statements and `JSONObject` -> `[String: Any]` conversion
|
Looks like there's a bunch of compilation errors on the codegen tests, I think you can probably find those pretty quick if you compile locally. I'll have to take a look at Carthage stuff separately, but as long as it compiles locally since carthage uses the underlying xcode project to compile, theoretically it should work correctly. |
| import Foundation | ||
|
|
||
| /// A reference to a cache record. | ||
| public struct CacheReference { |
There was a problem hiding this comment.
Apollo Android uses CacheReference too. I'd be in for renaming to EntityReference as we're still at a moment where we can rename stuff on Android. Feels like that would be more adequate. It's a reference to an Entity, not to a Cache. Or maybe ObjectReference ? (We don't have entity anywhere in the Android codebase)
There was a problem hiding this comment.
@designatednerd What's your opinion on the naming of this? Entity, Object, something else?
There was a problem hiding this comment.
@martinbonnin Is your point that it's a reference to an entity within the cache rather than to the cache itself? I think that's valid, but Entity by itself is so overloaded I might go with something like CacheEntityReference.
There was a problem hiding this comment.
Is your point that it's a reference to an entity within the cache rather than to the cache itself?
Yes it was! Rereading that, I'm certainly overthinking this. CacheReference sounds reasonable.
And now back to overthinking this... What's the difference between a CacheReference and a CacheKey ? Could we merge both somehow?
There was a problem hiding this comment.
My recollection is that the key is the key used to access stored data vs the reference is the actual stored data - @AnthonyMDev is that about right?
There was a problem hiding this comment.
I got curious and tried to merge CacheKey and CacheReference. Turns out it's working well: apollographql/apollo-kotlin#3157
The only reason I can think of having two different data structures in Kotlin is if we want to make CacheKey a value class that is inlined to a String for performance reasons. If we do that, we can't check the type of CacheKey anymore which means everything becomes a string and we can't serialize the CacheKey to their special String value.
But besides that (which we don't optimize at the moment and I feel should be an implementation details if we ever decide to) I actually like the reduced mental load and API surface.
There was a problem hiding this comment.
Hmmmmm. I'm going to look into this. Thanks for that @martijnwalraven
My recollection is that the key is the key used to access stored data vs the reference is the actual stored data - @AnthonyMDev is that about right?
No, the reference isn't the actual stored data. It's just a wrapper type for the CacheKey as a String. It indicates that the raw String data isn't just a regular raw a String property on the object, but is a String representing the reference to the cache key of another object.
There was a problem hiding this comment.
Gooooot it, thank you for clarifying
calvincestari
added
the
codegen
There is a lot in this PR, so bear with me. This makes a few changes to start enabling the work on the Codegen rewrite.
ApolloCoretoApolloUtilsApolloandApolloCodegenLib. It provides common utility functionality that can be helpful for any of our code.Utilitiesfolder ofApollowill likely be moved here.ApolloModelslibrary.Apolloand by any generated models that are generated using the new codegen.ApolloModels/CodegenV1folder. These are needed for future development work, but don't need to be included in distributed library until version 1.0.0 with the new Codegen.ReferencetoCacheReferenceEntityReference?InputValuerefactoredJSONEncodabledirectly.JSONEncodableto be inApollo, butInputValueneeds to be inApolloModelsSo the evaluation functions were moved into an extension that is inApollo.JSONEncodableand if they don't (they always should though), it throws an error.Dictionary(_ elements: uniquingKeysWith:)JSONValueMatcherto wrap theequals(_: Any, _: Any) -> Boolfunction. You now need to explicitly callJSONValueMatcher.equals(x, y)and we get that function out of the global namespace.Matchablemoved intoApolloTestSupportJSONDecodingErrorand only for test matchers. Moved theextension JSONDecodingError: MatchableintoApolloTestSupportas well.compactMapValues.OperationMessageby making themessageproperty aletand computing it once in the initializer.