Make it possible to extend ApolloCache<NormalizedCacheObject> in a InMemoryCache compatible way #12040
Conversation
👷 Deploy request for apollo-client-docs pending review.Visit the deploys page to approve it
|
|
|
Could you please explain the motivation a bit here? |
|
@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? |
msand
force-pushed
the
make_inmemorycache_compat_possible
branch
from
bea123f to
0510eab
Compare
| expect(merged["unique" + i].value).toBe(i); | ||
| expect(source["unique" + i]).toBe(merged["unique" + i]); | ||
| expect(merged[`unique${i}`].value).toBe(i); | ||
| expect(source[`unique${i}`]).toBe(merged[`unique${i}`]); | ||
|
|
||
| expect(merged.conflict).not.toBe(source.conflict); | ||
| expect(merged.conflict["from" + i].value).toBe(i); | ||
| expect(merged.conflict["from" + i]).toBe(source.conflict["from" + i]); | ||
| expect(merged.conflict[`from${i}`].value).toBe(i); | ||
| expect(merged.conflict[`from${i}`]).toBe(source.conflict[`from${i}`]); | ||
|
|
||
| expect(merged.conflict.nested).not.toBe(source.conflict.nested); | ||
| expect(merged.conflict.nested["nested" + i].value).toBe(i); | ||
| expect(merged.conflict.nested["nested" + i]).toBe( | ||
| source.conflict.nested["nested" + i] |
There was a problem hiding this comment.
Ah, I missed that these were in a test file - in runtime code they would likely have increased size.
|
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 |
|
I've got to admit, I'm still a bit confused: why are you trying to 100% match If there is anything causing why you want to 100% mirror If on top of that, you are 100% sure that you also match all internals, you could in the end |
|
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: msand/apollo-cache-hermes#2 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 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 As for functions in userland code that specify 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. |
|
Ok, I think another option to making these private methods public is to make them properly private, i.e. using the 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. |
|
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>> |
|
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 ;)
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 :) |
|
@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? |
|
Removing a From the TS handbook: You could mark your implementation as 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 Really, I think you're overthinking this - in your tests, just do a |
msand
force-pushed
the
make_inmemorycache_compat_possible
branch
from
668f118 to
511458a
Compare
|
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? |
…MemoryCache compatible way
msand
force-pushed
the
make_inmemorycache_compat_possible
branch
from
511458a to
475ff2f
Compare
As you can see in our shipped types, the TypeScript compiler already removes the 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 The moment we would remove the We really cannot do this, I'm sorry. |
msand
force-pushed
the
make_inmemorycache_compat_possible
branch
from
e3651ec to
014770f
Compare
|
@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? |
msand
force-pushed
the
make_inmemorycache_compat_possible
branch
from
014770f to
5522dcb
Compare
|
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. |
|
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? |
msand
force-pushed
the
make_inmemorycache_compat_possible
branch
from
5522dcb to
0ac4b3e
Compare
msand
force-pushed
the
make_inmemorycache_compat_possible
branch
from
0ac4b3e to
fbd6be1
Compare
|
That would always require a lie to TypeScript and potentially break user expectations.
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 |
|
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. |
|
@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. |
|
@msand do you have examples where users are having difficulty adopting Hermes specifically because they are trying to migrate away from 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 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 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. |
|
@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? |
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 Again, the rest of Apollo's core works with - function addTypePolicy(cache: InMemoryCache, ...)
+ function addTypePolicy(cache: Hermes, ...)If this satisfies those areas where devs use Is it too much to ask users to simply update their types once you have those features implemented? |
|
@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 |
|
@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? |
|
@jerelmiller Not sure, but it seems you haven't read my comment where I say this:
|
|
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. |
|
@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.
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 Really it comes down to the fact that we'd prefer to continue using the
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 In practice we just haven't seen many people use the
Edit: Apologies, just realized I'm using |
|
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 |
|
I want to add to that that Please, can we step back from the hypothetical scenario of "someone might want to replace it, but plastered their codebase with This is highly unrealistic, and if it happens, and we get good feedback around it happening, we can still do it then instead of prematurely optimizing for that now. We are really talking about "making DX worse for hundreds of thousands of users to make DX better for one hypothetical user" at this point 😞 If you really still want to be a drop-in replacement for that scenario, please consider this workaround, which I've now already mentioned twice. At that point it will be up to you to maintain compatibility. Quoting it here again for visibility: class MyMemoryCache extends ((class {}) as typeof InMemoryCache) {
// your implementation
}
const cache = new MyMemoryCache();
let foo: InMemoryCache = cache;You can do this today without any changes on our side. |
|
Hmm, let me ask you this, if I implement feature parity in Hermes, such that both types and run-time behavior are compatible, do you think typescript should give errors in that case? If you think it should error, then I no longer see any point in working further on adding features to Hermes, and will just fix the handful of relevant tests that remain for the apis we already use, and then stop any further investments in Hermes. At least I do not desire to make typescript lie, and will not introduce a single typecast into Hermes. Would you accept prs where incorrect typecasts are used in production code? Are you really serious about this suggestion? The whole point of removing these private modifiers is to get typescript to accurately report if the interfaces are compatible or not, rather than lie and claim they're not compatible when in fact they are. Well at least I can be happy I made this pr to make sure there is sufficient reason to work any further on Hermes before I put any effort into feature parity. |
|
Can I ask this one more time?
I'd like to understand why you do or don't see this as an option? |
|
Big enough to matter, most people are lazy, and irrational, and afraid, and oh, did I mention lazy? One error too much from typescript and their tired brain switches to wanting to go drink a beer in the bar or get intimate with their partner, or play a game or whatever, you get the point. If the types actually agree and typescript says it doesn't, then the cost of a few billion neuron firings to fix it will be too much for some fraction of developers. And I suspect attempting to get typescript to change their policy around how to decide if two types are compatible based on having private modifier or not would probably be received even worse than this attempt here. Either way, the apollographql community doesn't seem receptive of the kinds of efforts I'm proposing. So I guess it is time to move on to the next strategy. Time to replace apollo-client and graphql with a thin binary protocol and minimal api surface, our use case doesn't actually need most things from graphql and can be handled with much better performance in other ways. Au revoir 😗 |
|
@benjamn Is this the kind of direction you want apollographql/apollo-client + community to go? |
|
@msand I'm sorry we couldn't come to a solution that works for both of us. To be quite frank, you are asking us to add and support a lot more work and that is something we are unwilling to do for a single use case. I know you don't see it that way and unfortunately this is where we have to agree to disagree. I think @phryneas and I have done what we can to try and work with you, but at this juncture it seems you are unwilling to do the same. |
|
Hmm, now I'm confused, in what way do I seem unwilling to work with you? I've provided every implementation I can think of that could satisfy typescript in the case that the types are the same. And working thru implications in terms of documentation and communication. It seems where we disagree is about how much it matters if users have to do more than just replace InMemoryCache in one place with Hermes to try it out. Sure, if there's no other way than getting typescript to change how they treat the private and protected modifier, then I'll give up, but certainly not for lack of trying to work with you to get typescript to give accurate claims. At least it seems strange to me to reject community efforts to create alternative implementations. These are open source projects which can benefit each other no? If another implementation finds a more fruitful strategy, then that can probably be adapted into the other as well, raising performance for everybody. I don't know, seems like it should be a win-win all around. A more performant alternative cache should just bring more users to apollo-client, I can't see how it would compete with it, given that it requires apollo-client to work. |
|
Actually, it seems that typescript should have |
|
This will be my last reply. We've provided plenty of reason why we don't see this as a viable solution for the issue you are proposing, and you seem unwilling to acknowledge our concerns. Statements such as
tell me you're done listening and unwilling to work with us further. You're more than free to use what tool suits you best, but these types of threats do nothing to move the conversation forward. In the end, its us that bears the responsibility of these changes, both from a technical perspective and from a support perspective. Our experience in open source informs our reasoning for why we have the concerns we do.
Responding with concerns about your proposed solution != rejecting community efforts to maintain alternative implementations. We absolutely love that alternative cache implementations exist and encourage those that want to break away from To be clear, we love contributions to the library, but a contribution does not guarantee that change gets merged. Apollo Client is MIT licensed. You're more than free to fork it and update |
You're really trying to move goalposts here: This is going to be a never-ending game: After Worse yet: what would you demand of us the moment we shipped two different cache implementations that both fulfill I do not think discussing things here will serve any more purpose:
As a result, I'm going to close this PR. |
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.