Make it possible to extend ApolloCache<NormalizedCacheObject> in a InMemoryCache compatible way

[msand]

This pull request enables extending ApolloCache in such a way that a function having parameters of type InMemoryCache can accept these alternative implementations as well. I hope these changes are alright, or that we can find some other way to enable this use case.

[netlify[bot]]

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	fbd6be184c8cecfad5c4062b939a328905ed6932

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: fbd6be184c8cecfad5c4062b939a328905ed6932

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[phryneas]

Could you please explain the motivation a bit here? This PR does a lot of things that seem unrelated to each other - including making a lot of private functions public, which we just consider to be implementation details and might change at any given moment and code style changes which significantly increase the size of the transpiled code.

[msand]

@phryneas Good day Sir! Sure, I've been maintaining a fork of apollo-cache-hermes for about a year now, and currently finishing up the last compatiblity differences between it and InMemoryCache. To do that, I've copied the apollo-cache repo into it, replaced InMemoryCache with Hermes in all the tests, fixed all the type issues, and now making all the remaining tests pass.

To get the types to match, I had to remove the private and protected modifiers, otherwise typescript won't consider the implementations to be compatible, even if the signatures match exactly otherwise. After all the tests pass, I'd like to make a pull request which brings Hermes into apollo-client, as an alternative high performance cache. I'm sure there's several things from apollo-client that could be reused inside of it, and ideally there would be more people maintaining and improving the implementation. I've recently doubled the performance by doing a core data structure change.

The type changes in the tests aren't strictly necessary, they just make it easier to satisfy typescript inside the hermes project in this specific setup, but those shouldn't affect bundle size either. And looking at the changes now, I can't really see anthing that would have a large impact on bundle size, I think this should be non-breaking and backwards compatible type related changes only, or am I missing something which would end up in a js file?

[phryneas]

Generally, you can find a diff of the build results here: https://github.com/apollographql/apollo-client/actions/runs/10594626400

Is it accidental that you are not exporting Cache anymore?

[phryneas]

I've got to admit, I'm still a bit confused: why are you trying to 100% match InMemoryCache? The public interfaces for caches is ApolloCache - if you already match that, everything should be good. InMemoryCache is one possible implementation, but it doesn't have to be replicated fully. Replicating our implementation like that would restrict you for no good reasons - your implementation might be completely different and not have any need to match all private functions, it might even make your implementation more complicated than necessary for what you want to do.

If there is anything causing why you want to 100% mirror InMemoyCache instead of ApolloCache, that might be something more important that we would need to tackle on our side. We don't want to lock people in a 1:1 copy of InMemoryCache.

If on top of that, you are 100% sure that you also match all internals, you could in the end export YourCacheImplementation as InMemoryCache, but I have to be honest, I might still be missing the reason to do that in the first place here.

[msand]

The linter was complaining about the Cache import being type only (it only exported a namespace), so fixing that caused the export snapshot to fail, so had to remove it, if you check the build diff you can see it only exported an empty object.

Main reason to replicate it is to be able to reuse all the tests that exist in apollo-client, they've already exposed several things I want to have fixed. In practice I won't replicate all internal behavior, only test all the parts which are of relevance. So in fact, I don't strictly need these changes merged. If necessary then I can also keep the current setup where I replicate apollo-client inside the hermes repo and make any necessary changes to ensure feature parity of the relevant parts.

Another motivation to have the interfaces match, is that it seems quite likely that some functions are specified to have InMemoryCache as parameters, and having typescript happy without any typecasts improves type checking and dx in general. Making the types match already made me realize the possibility of a significant simplification in the types of Hermes as well, which is in the wip branch as well: https://github.com/msand/apollo-cache-hermes/pull/2

So, mostly this change just makes it easier to maintain alternative ApolloCache implementations which reuse the InMemoryCache and other apollo-client tests.

[phryneas]

So, mostly this change just makes it easier to maintain alternative ApolloCache implementations which reuse the InMemoryCache and other apollo-client tests.

I'm sorry, but if that's the main motivation please continue to maintain these changes on the side, or casting your implementation as InMemoyCache in your codebase where needed.

A change like this really locks us into implementation details that we would at some point want to change, and will make our work a lot more difficult. After this, every change to a function that currently is private would be considered breaking and require a major release.

As for functions in userland code that specify InMemoryCache instead of ApolloCache: if they want to swap in another cache implementation, it should be no problem to also swap these types on their side.

Bottom line: thank you for your work of actively maintaining https://github.com/msand/apollo-cache-hermes - it's very valuable to the community! I really don't want to seem dismissive in any way, but I don't think that these changes are something we can commit to.

[msand]

Ok, I think another option to making these private methods public is to make them properly private, i.e. using the #fnName() {} and #member = somevalue; syntax. I think then typescript won't complain about any interface mismatch either. Wdyt? The delete and invalidate modifiers still need to be exported, but that seems like it should be completely safe. This doesn't expose any private implementation details which aren't already exposed or apollo is not ready or wanting to commit to atm.

And how about merging Hermes into apollo-client later on? What would be required to get that to happen? I suspect a lot of people don't know about Hermes, or only know the upstream repo which is lacking a lot of functionality / valuable apis and rejecct it due to that and the repo being stale for a long time. Having it available in apollo-client as a tree-shakeable sub module / not exposed in the main exports wouldn't pollute the normal bundle size, while getting more exposure and increased likelihood of a larger community maintaining the implementation.

[msand]

Scratch that, # syntax has same issue:

TS2322: Type Hermes is not assignable to type InMemoryCache
Property #broadcastWatch in type Hermes refers to a different member that cannot be accessed from within type InMemoryCache
policies.ts(207, 3): The expected type comes from property cache which is declared here on type
FieldFunctionOptions<Record<string, any>, Record<string, any>>

[phryneas]

Yeah, I fear that's not really an option as it would also add a lot of friction - it would transpile differently and might cause problems with Vue - as far as I know that has some APIs that really don't like "real private" JS properties. It also makes it more difficult for us to work with it - sometimes we do use the escape hatch of runtime ;)

And how about merging Hermes into apollo-client later on? What would be required to get that to happen?

The moment we merge something into the official Apollo packages, we commit on keeping it maintained for a very long time - and the team already has their hands full, so I don't think we can make that commitment.

What we could do is mention alternative cache implementations in the Docs - if you see a good place to put that into our documentation, maybe something similar to https://www.apollographql.com/docs/react/integrations/integrations/ or somewhere in https://www.apollographql.com/docs/react/caching/advanced-topics, I think we'd be open for a PR there :)

[msand]

@phryneas I have a version which just removes the private and protected modifiers now, doesn't make them public. In practice all of these are accessible using string property indexers already no? At least given that they're not part of the public interface, they shouldn't count as any more or less stable than before this way no?

[phryneas]

Removing a private or protected identifier really just implicitly adds the public keyword from a TypeScript perspective. We cannot do that.

From the TS handbook:

---

You could mark your implementation as InMemoryCache-compatible with something like this

class MyMemoryCache extends ((class {}) as typeof InMemoryCache) {
  // your implementation
}

const cache = new MyMemoryCache();

let foo: InMemoryCache = cache;

but you'd have to be very careful - we could add additional functionality to InMemoryCache at any time, and then it would look like you had implemented that, even if you had not.

Really, I think you're overthinking this - in your tests, just do a as any as InMemoryCache, or // @ts-ignore. Doing that in tests is perfectly fine!

[msand]

Hmm, seems this is mostly a matter of style no? Could use the explicit public modifier to indicate stable / public apis you wish to support. Can't see any technical reason why this wouldn't be possible at least, or am I missing something? Afaik, the only observable change is that instead of having to use bracket notation / indexers to access them, it is possible to use direct property access as well if one so desires?

[phryneas]

Can't see any technical reason why this wouldn't be possible at least, or am I missing something?

As you can see in our shipped types, the TypeScript compiler already removes the public keyword from public properties, because it's implied.

This is also not about the philosophical difference if we write it out or not - VSCode would offer those in autocomplete nonetheless, as technically both the explicit and implicit public are exactly the same to TypeScript (and as a consequence, to the language server). Users wouldn't be able to notice any difference unless they manually looked at the shipped type definitions.

The moment we would remove the private or protected keywords, all of those would become part of our official API interface and would indicate that users could use them and rely on their stability. But they cannot, especially all private properties are implementation details we might change in any patch release.

We really cannot do this, I'm sorry.

[msand]

@phryneas Hello again :) So another strategy, no more publicly exposed methods or fields 😄 While keeping everything accessible where necessary. Would this be an acceptable direction?

[phryneas]

Please take a step back and look at the big picture here!

At this point you're asking us to do runtime code changes to our implementation so you can avoid any-casting in your tests! 😕

I have given you multiple suggestions on how you can solve this on your side.

[msand]

Well ignoring type issues in tests is one thing, that is quite fine. But I'd like to get it such that end users can provide instances of Hermes to any function that expects InMemoryCache. Is there really no way to make that happen in a way that satisfies you?

[phryneas]

That would always require a lie to TypeScript and potentially break user expectations.

• No matter what you do, instanceof won't do what you want.
• Even if we did all you are asking here, every property or function we added would either break your compatibility on a type level or on a runtime level. To keep that compatibility, we could never make any change to InMemoryCache again (and we even have PRs open for that right now!)/

If you're willing to make the lie on a type level, I already showed you how to do that.

Generally, I'd suggest that you take a step back there, too, and ask yourself why you want to do that in the first place. Either that user has wrong types in their code base (and should fix them), or they really want a InMemoryCache instance.

[msand]

Well instanceof saying that Hermes isn't an instance of InMemoryCache is fine, what I'm looking for is interface level compatibility at the public api level of InMemoryCache. One of the main reasons is to minimize friction in adopting Hermes, if any end user can use Hermes in all places where they currently use InMemoryCache (except instanceof checks), and without any typecasts or lies involved, then this removes a potential reason not to use Hermes.

If e.g. we would remove all the private and protected modifiers then no functionality would change and typescript would inform correctly if there is a mismatch of any sort. This seems like the most sensible way forward, as it doesn't break anything or have any impact on runtime. Also, as these fields and methods are already exposed using bracket notation, and the fact that using patch-package anyone can remove the modifiers, it doesn't actually affect what is exposed / potentially in use in the wild / expecting support or being ready to modify if changes happen.

Seems like just adding comments on the fields you wish not to support or keep internal / private, would probably have the desired setting of expectations for anyone using them. At least the case I'm thinking of here does not involve the user having wrong types nor wanting a InMemoryCache, but rather wanting to try Hermes without making any other changes to their codebase.

[msand]

@jerelmiller Well for now I don't know if anyone else even knows about my fork, I suspect most people looking at Hermes nowadays discard it immediately due to lack of maintenance and missing features. Driving these changes is the desire to make it as easy as possible to test Hermes, to maximize potential use and growth of community around its maintenance. One of the main ways to achieve that is to ensure that anywhere anyone would try to put Hermes, typescript is satistfied. And one of the main ways that works against it, is if replacing InMemoryCache with Hermes causes typescript to complain.

Other than that, it seems it is only down to performance and the fact that InMemoryCache only returns what the graphql query asks for, while Hermes provides the entire object from the cache. Only other thing that comes to mind would be if there are differences in functionality, but once all the features of the public api have parity at the test level, that shouldn't be a factor anymore. E.g. I'm looking to implement TypePolicies quite soon.

[jerelmiller]

@msand do you have examples where users are having difficulty adopting Hermes specifically because they are trying to migrate away from InMemoryCache? Having some understanding of what Hermes users are experiencing would really help understand what's driving your desire here.

---

I'll echo what @phryneas said though in that I don't think its a simple swap from one cache to the other. Not even Apollo Client's core knows about or uses the InMemoryCache type explicitly. It's very deliberate to use the base ApolloCache type because it allows you to swap out caches without having to modify code in a bunch of places.

I'm also just not sure that making the interface available would work the way you're expecting. For example, the following would break at runtime if you simply swapped caches, despite the fact that TypeScript is satisfied:

function addTypePolicy(cache: InMemoryCache, typePolicy: TypePolicies) {
  cache.policies.addTypePolicies(typePolicy);
}

const cache = new Hermes();

addTypePolicy(cache, {...})

The interface alone is not enough to satisfy the feature set that InMemoryCache provides, and I doubt that you want to maintain a 1:1 working implementation of these features in Hermes. The only places we've seen where users are specifically working with the InMemoryCache instance itself is cases where you've split out some of the configuration to multiple files (such as lazy type policy registration). Again, the interface alone would not be enough to satisfy this use case as switching to Hermes would still require code changes since it would require moving away from type policies to work.

---

Again, having some user feedback to look at would really help us understand the desire here. I just don't see how we can adopt this change otherwise.

[msand]

@jerelmiller Yes, I'm quite aware of the difference between interface and implementation, I've been doing software development for more than twenty years and have a masters degree in computer science, and currently CTO of a software startup. I do understand what my words imply. Currently my fork has no other users, so there is no way I can provide user feedback other than my own, and it seems that does not count here?

[jerelmiller]

one of the main ways that works against it, is if replacing InMemoryCache with Hermes causes typescript to complain.

I actually think this is a good thing! Otherwise users might expect that certain features work with Hermes that aren't actually implemented (again see the addTypePolicies example).

Again, the rest of Apollo's core works with ApolloCache directly specifically so that we have a guaranteed set of types and runtime behavior regardless of what cache you're using. Breaking the guarantee of runtime behavior to satisfy TypeScript feels dangerous. Besides, once you have some of these features implemented, I don't think its a big ask for users to update their types:

- function addTypePolicy(cache: InMemoryCache, ...)
+ function addTypePolicy(cache: Hermes, ...)

If this satisfies those areas where devs use InMemoryCache features is a much better guarantee that the runtime behavior is satisfied.

Is it too much to ask users to simply update their types once you have those features implemented?

[jerelmiller]

@msand I'm not questioning your ability. Please don't go there. This is not a credential war.

I'm just trying to understand your ask here to find a solution. What you're asking us to do is maintain a new set of guarantees in InMemoryCache that hamstring future changes for us. You understand that right?

[msand]

@jerelmiller Oh, seems I misunderstood, it is just that pointing out "The interface alone is not enough to satisfy the feature set" has some implied assumptions around ability / understanding of software, thought perhaps it wouldn't hurt to make sure you know what kind of terminology you can discuss with me, and, that you can safely assume I understand the difference between typescript claiming two interfaces to be compatible vs runtime behavior.

I don't see how it would affect any guarantees you maintain nor hamstring any changes. If the fields and methods are documented as internal and say not to depend on any features related to it and to expect changes at any point, then how is it different from now? The fields and methods are already accessible and possible to have dependencies on, right?

[msand]

@jerelmiller Not sure, but it seems you haven't read my comment where I say this:

E.g. I'm looking to implement TypePolicies quite soon.

[msand]

No matter how I've tried, I can't seem to come up with anything else that this would affect than the use case I'm advocating for. What you choose to support is up to what you communicate, and that is mostly in the form of documentation and websites, but also intellisense, and if that makes it explicit then I can't see where there would be any confusion.

[jerelmiller]

@msand apologies that statement came across as questioning your understanding of software. Definitely not my intention here. This was more just me speaking out loud about the potential problems I see moving this direction. I tend to verbalize my whole thought process, but I recognize sometimes this comes across as me trying to explain something somebody already knows, even though in my head its a means to provide context.

If the fields and methods are documented as internal

We don't publish documentation for internal properties/functions in our public documentation (as you can imagine), so really this leaves JSDoc as the means to communicate internal functions.

As much as I'd love for everyone to look at documentation, as soon as a property or function is available via autocomplete in your editor, devs will use it. We would hope that each of them take the time to look at the documentation, but we can't guarantee that, so we use the private modifier for that reason. It's at least a very strong signal that we're allowed to completely modify/rename/remove those APIS because we can't guarantee them as stable.

Really it comes down to the fact that we'd prefer to continue using the private modifier as the signal for instability rather than documentation because TypeScript helps us here. Moving these properties to public puts us on the hook to maintain these as public APIs (which means non-breaking changes only) and we don't want to do that for internal implementation details.

it seems you haven't read my comment where I say this:

I saw this after I posted my last comment, you just happened to hit "Submit" before I finished my reply 😆.

I'll echo again though, I think its better practice for users to switch the types and have the Hermes cache guarantee InMemoryCache runtime APIs exists before it can be adopted rather than simply conform to the InMemoryCache interface. It's at least a signal to those users which of the InMemoryCache APIs are stable in Hermes and can be used the moment they install it. Discovering at runtime that an API doesn't work might bring about some frustration. Trust me, you don't want to put out those fires in your issues 😂 (and props to you if you love handling these types of issues!)

In practice we just haven't seen many people use the InMemoryCache type on its own in a bunch of places throughout their apps (granted we have seen a very small sample size). Typically users access it through client.cache and this is reported as the ApolloCache type.

---

~All this aside, I wanted to discover for myself how TypeScript might handle a situation like this. I put together this TypeScript playground, but it seems like this might not work? I see the following error:~

Argument of type 'B' is not assignable to parameter of type 'A'.
  Types have separate declarations of a private property 'privateProperty'.

~Have you had success getting Hermes and InMemoryCache to work together with the changes you've proposed in this PR?~

Edit: Apologies, just realized I'm using private here, which is what this PR is all about.

[jerelmiller]

Let me ask again though, do you think its a big lift to ask users to do a find and replace in their codebase from InMemoryCache to Hermes?