Make ApolloStore an open extensible class, and its previously-internal dependencies public

[lincolnq]

(This is in reference to apollographql/apollo-ios#3461, which I filed earlier today.)

The goal of this PR is: Make it easier to customize behavior of the Apollo normalized cache.

Currently, ApolloStore is a concrete class depended-on by lots of Apollo internals. It's not marked open, nor is it a protocol, so in practice there is no way without modifying Apollo code for applications to customize the store behavior.

With this PR, it is still necessary to use @_spi(Execution) for overriding particularly fiddly bits of ApolloStore behavior. I think this is probably a good thing to help users understand that they're digging deep into the internals, while still allowing us to customize the behavior we want.

Feedback is welcome. In particular I'm not sure whether to do what Android does and make ApolloStore a protocol and rename the current ApolloStore to DefaultApolloStore. I can make that change too if desired.

---

Feel free to skip the below, but here's some more context about the why:

My specific use-case is one that has been touched-on/requested a number of places (apollographql/apollo-ios#3319, apollographql/apollo-ios#892, apollographql/apollo-ios#3216: make the Apollo cache accept missing values for nullable fields as null, rather than failing to parse them.) While Apollo team has resisted adding a flag, if ApolloStore were extensible I could implement the behavior I want myself without forking all of apollo-ios.

This example is what I was trying to write, impossible (as far as I can tell) without making ApolloStore extensible. It's just an ApolloStore that passes .allowForOptionalFields to the GraphQLSelectionSetMapper during load:

import Foundation
import ApolloAPI
@_spi(Execution) import Apollo

public class MyApolloStore: ApolloStore {
    override open func load<Operation: GraphQLOperation>(
        _ operation: Operation,
        callbackQueue: DispatchQueue? = nil,
        resultHandler: @escaping GraphQLResultHandler<Operation.Data>
    ) {
        withinReadTransaction({ transaction in
            let (data, dependentKeys) = try transaction.readObject(
                ofType: Operation.Data.self,
                withKey: CacheReference.rootCacheReference(for: Operation.operationType).key,
                variables: operation.__variables,
                accumulator: zip(GraphQLSelectionSetMapper<Operation.Data>(handleMissingValues: .allowForOptionalFields),
                                 GraphQLDependencyTracker())
            )

            return GraphQLResult(
                data: data,
                extensions: nil,
                errors: nil,
                source: .cache,
                dependentKeys: dependentKeys
            )
        }, callbackQueue: nil, completion: resultHandler)
    }
}

[netlify]

👷 Deploy request for eclectic-pie-88a2ba pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	5b2f32b

[netlify]

👷 Deploy request for apollo-ios-docc pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	5b2f32b

[svc-apollo-docs]

✅ Docs Preview Ready

No new or changed pages found.

[AnthonyMDev]

Thanks for this PR! I haven't had time to review this in detail yet, but I wanted to let you know that I'm working on Apollo 2.0 to support the Swift 6 Concurrency Model, and one of the big features I'm currently planning on designing is this.

Once I have a better idea of the design, I'll be updating the 2.0 RFC with more information. I will definitely be looking at this PR in greater detail for inspiration as I iterate on the design for this in 2.0.

[lincolnq]

Ok - in case you haven't gotten a chance to look yet - this change is fairly small & (as far as I can tell) there's very little in here that should impact anything related to Swift 6 Concurrency specifically, although obviously I don't have much context on your goals related to that.

I'm inferring from your message that this change is not likely to be merged before Apollo iOS v2? Right now I'm maintaining a fork to add this feature, but I don't really want to keep doing that -- if there's anything I could do to simplify/reduce the effort to look at this and maybe get it merged sooner, I'd love to know what.

[AnthonyMDev]

Thanks for following up @lincolnq. I just took a loot at the PR.

I'd like to allow my colleagues @calvincestari and @BobaFetters the chance to comment on this, but my inclination is towards rejecting this PR. While I do understand your needs are valid here, this is exposing a lot of APIs publicly that are not really intended for public consumption.

We do want to expose a lot more extensibility in the caching mechanisms, but to be honest, a lot of these APIs are not really well designed, especially not for extensibility. We take great care to design our public APIs well and document them thoroughly. All of these internals are very difficult to use and expose lots of possibilities for bugs if not used correctly.

A lot of these internal pieces are not set up for extensibility. They are interacting with other internals that expect them to work exactly how they currently work. If you are able to manage your customizations in a way that doesn't break anything, that's great, but unless we do a lot of work to account for possible changes in behaviors people could create, this isn't appropriate to actually be public.

From experience, every time we expose a new feature or customization point, we inevitably get a number of new requests from users who want to use it in different ways we didn't account for. These APIs are already very sensitive to change and weren't designed with end users in mind. I'm concerned this PR would just open a big can of worms for us.

This also makes it a lot harder for us to make improvements or changes in the future without making breaking changes. Some of the things you are exposing have changed internally quite a lot, and I expect will continue to change in ways that would be breaking if they were public APIs. Keeping them private allows us to safely iterate on them.

Right now I'm maintaining a fork to add this feature, but I don't really want to keep doing that

I do understand the frustrations that causes with staying up to date with other changes to the repo, but unfortunately, I'd recommend you keep using your own fork for these changes. Part of the value of open source is that you are welcome to do anything you want with the code, but if we make this part of the public API, we are then beholden to support users who are using them in ways that we are not currently prepared to support.

The right approach to this is to design a better, more extensible public API surface that we actually feel good about exposing to users and are prepared to support going forward. And that is something we're working on for 2.0 and beyond.

[lincolnq]

Ok, sad to hear it but glad we're having the discussion.

Apollo Android uses an interface for ApolloStore and then an internal impl of DefaultApolloStore.

It's a lot less useful for my purposes, but I would still have less 'surface area' to copy, if ApolloStore was similarly a protocol instead of a concrete class. I haven't looked into how difficult it would be, but it would at least be possible, if iOS made the same change. Would you be more open to a PR that made ApolloStore into an interface (protocol)?

[calvincestari]

I agree with @AnthonyMDev. I'm also not comfortable with the number of internal APIs that need to be exposed, particularly around the executor. I would prefer taking a look at redesigning this with 2.0 or shortly thereafter with the idea of extensibility from the start. Making the proposed changes might open what we have now but it's not what the design was intended for which increases the maintenance cost.

We do appreciate the contribution @lincolnq but for now I'm going to close this PR.