experimental ApolloClient.getMemoryInternals helper
#11409
Conversation
🦋 Changeset detectedLatest commit: c3ad505 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
| */ | ||
| export const getApolloClientMemoryInternals = | ||
| __DEV__ ? | ||
| (_getApolloClientMemoryInternals as RemoveThis< |
There was a problem hiding this comment.
For future me, a comment about RemoveThis would be helpful here. Something like:
Since InMemoryCache inherits from ApolloCache, TS complains about the incompatibility of
this, and so while it's useful to define the type ofthisas eitherInMemoryCacheorApolloCachefor the purpose of the actual implementations in_getApolloCacheMemoryInternalsand_getInMemoryCacheMemoryInternals, we removethisbefore exporting to avoid the TS conflict on assignment to their respective prototypes.
| ].filter((x) => x != null); | ||
| } | ||
|
|
||
| // removeTypenameFromVariables getVariableDefinitions |
| persistedQueryHashes: 1, | ||
| }, | ||
| { | ||
| getVariableDefinitions: 0, |
There was a problem hiding this comment.
I'd be curious if you'd be interested in a sort of namespacing or identification for these links. Since these memory profiles will be useful for dev tools, its going to be really difficult to correlate these numbers with the links they belong to. Perhaps something like this?
| getVariableDefinitions: 0, | |
| 'removeTypenameFromVariables.getVariableDefinitions': 0, |
or
| getVariableDefinitions: 0, | |
| removeTypenameFromVariables: { | |
| getVariableDefinitions: 0, | |
| } |
or maybe it makes sense to add a name property 🤔
| getVariableDefinitions: 0, | |
| name: 'removeTypenameFromVariables', | |
| getVariableDefinitions: 0, |
Thoughts? Or does this clutter this up too much?
There was a problem hiding this comment.
Let's wait for the pattern we come up over in the cache PR then we'll adjust here :)
| link?.getMemoryInternals?.(), | ||
| ...linkInfo(link?.left), | ||
| ...linkInfo(link?.right), | ||
| ].filter((x) => x != null); |
There was a problem hiding this comment.
| ].filter((x) => x != null); | |
| ].filter(Boolean); |
might also be a nice shortcut that saves a couple bytes 🙂.
There was a problem hiding this comment.
I had that, but it also removed 0 values in the DocumentTransform use case further down (which just returns a number, not an object).
So we definitely need this down there. I can see if extracting it out can save a few bytes so at least we don't repeat the function definition. (Although we are talking dev bundle size anyways ^^)
There was a problem hiding this comment.
Results in the same size, but slightly nicer: e283de3
| }, | ||
| links: linkInfo(this.link), | ||
| queryManager: { | ||
| Transforms: this["queryManager"]["transformCache"].size, |
There was a problem hiding this comment.
| Transforms: this["queryManager"]["transformCache"].size, | |
| transformCache: this["queryManager"]["transformCache"].size, |
I'm curious on the decision to name this as Transforms while the rest of the entries are lowercase and typically named after the property they are pulling from. Would it make more sense to call this transformCache?
There was a problem hiding this comment.
The related cache configuration option is currently called queryManagerTransforms, so it seemed to make sense to have this as queryManager.Transforms instead of a completely different name.
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Opening this one for feedback, and to see how feasible it is to have a helper like this.
Is this possible without adding a lot of bulk to the prod bundle size?
Checklist: