feat(NormalizedCache): use a Map-like cache abstraction

[niieani]

This feature enables a swappable cache implementation. For example, you might want to use Map, which is faster than writing keys to an Object. This also allows for custom use cases, such as emitting events upon .set() or .delete() (think Observables), which was otherwise impossible without the use of Proxies, which were only available in ES6.

First PR to close #2293 (another part would be to open up the Array storing optimistic updates).

The change is as non-breaking as possible. Unless you passed in the store to one of the apollo-cache-inmemory functions, such as: writeQueryToStore or writeResultToStore, there are no changes necessary. If you did access the cache's functions directly, all you need to do is add a .toObject() call — see the changes to the tests for an example.

Looking forward to your reviews @jbaxleyiii and @kamilkisiela!

Checklist:

• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests
• If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

[apollo-cla]

@niieani: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[apollo-cla]

	Warnings
⚠️	❗ Big PR

	Messages
📖	Please add your name and email to the AUTHORS file (optional)

Generated by 🚫 dangerJS

[jbaxleyiii]

Deploy preview ready!

Built with commit 07c9609

https://deploy-preview-2362--apollo-client-docs.netlify.com

[codecov]

Codecov Report

Merging #2362 into master will decrease coverage by 0.07%.
The diff coverage is 84.11%.

@@            Coverage Diff             @@
##           master    #2362      +/-   ##
==========================================
- Coverage   86.04%   85.96%   -0.08%     
==========================================
  Files          36       39       +3     
  Lines        2049     2145      +96     
  Branches      490      500      +10     
==========================================
+ Hits         1763     1844      +81     
- Misses        277      292      +15     
  Partials        9        9

Impacted Files	Coverage Δ
packages/apollo-cache/src/cache.ts	68.42% <ø> (ø)	⬆️
...ackages/apollo-cache-inmemory/src/readFromStore.ts	92.5% <ø> (ø)	⬆️
packages/apollo-cache-inmemory/src/types.ts	100% <ø> (ø)	⬆️
packages/apollo-cache-inmemory/src/mapCache.ts	100% <100%> (ø)
packages/apollo-cache-inmemory/src/index.ts	100% <100%> (ø)	⬆️
packages/apollo-cache-inmemory/src/writeToStore.ts	94% <100%> (+0.12%)	⬆️
...kages/apollo-cache-inmemory/src/fragmentMatcher.ts	89.28% <100%> (ø)	⬆️
...ckages/apollo-cache-inmemory/src/recordingCache.ts	74.35% <74.35%> (ø)
packages/apollo-cache-inmemory/src/objectCache.ts	80.76% <75%> (ø)
...ackages/apollo-cache-inmemory/src/inMemoryCache.ts	81.96% <85.71%> (-0.3%)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7aa8cff...07c9609. Read the comment docs.

[jbaxleyiii]

@niieani I'm super excited about this! I'm going to label it as post 2.0 though since I don't want to introduce quite this large of a change right before launch. I'll be able to give it an indepth review next week!

[jbaxleyiii]

Since the normalized store is specific to apollo-cache-inmemory, this would be better there, instead of part of the base cache api

[niieani]

For a moment there I thought you're commenting about StoreValue, which I didn't touch. 😃
Fixed!

[jbaxleyiii]

@niieani This is looking fantastic! A minor request for a file move, but otherwise I'd like to move this into a testing phase!

Would you be able to describe some of the usages you have developed from this change, as well as add a couple new tests for using something like Map as the cache base?

After that, I'd like to cut a 2.1.0@next to give you (and others) the chance to try this out and profile it performance wise. Essentially I want to make sure the added weight / complexity of these changes are worth it for the possible performance gain. If Map is fasters, I think we should adjust this PR to make it the default storage for the InMemoryCache.

Looking forward to working with you to ship this! 💪

[kamilkisiela]

@niieani I applied your changes and created an ApolloCache that uses @ngrx/store.

https://github.com/apollographql/apollo-angular/tree/1.0-ngrx/packages/apollo-angular-cache-ngrx

It still work in progress, more a proof of concept but it looks so goooooood.

[niieani]

Hey @jbaxleyiii! Thanks for your review. I've moved the typings as you requested and added an additional implementation of MapCache (not used anywhere by default). Testing turned out to be simple, since I could just reuse all the tests and mock out the type of cache they all are using. :)
Note that I've not re-exported MapCache from index.js because that would prevent tree-shaking, at least until pureModule flag handling ships in Webpack 4.

Another thing: I originally wanted to do class MapCache extends Map, however that does not work in TypeScript (or babel) when the target is es5. Hopefully having the Map as a property of the MapCache won't be a big performance issue.

While adding the MapCache and running all the test suites with it, I've noticed a few failing ones (mainly: "will preserve equality with custom resolvers"). That was odd.

It turned out dataId can sometimes be a number, when the value is set, but read as a string, when the value is being retrieved (!). A side effect of the original, Object-based store was that indexer properties always have to be strings, so even if the code did cache[number], the JS engine would coerce that number to a string, and thus getting that same dataId (whether via a number or a string) would resolve to the proper underlying value. To solve this issue, I've added a helper function getNormalizedDataId, which ensures that dataId is in fact, not a number.

If this is a bug, and not a feature, then it should probably be corrected at the source, not in the implementation of the cache. It might also just be a problem with the test itself that I'm not seeing?

apollo-client/packages/apollo-cache-inmemory/src/__tests__/diffAgainstStore.ts

Lines 838 to 856 in 7aa8cff

	const listResult = {
	people: [
	{
	id: '4',
	name: 'Luke Skywalker',
	__typename: 'Person',
	},
	],
	};
	const itemQuery = gql`
	{
	person(id: 4) {
	id
	name
	__typename
	}
	}
	`;

The result contains id: '4' as a string, though the query specifies the id variable as a number: person(id: 4). Could that be it? I saw that various tests sometimes use strings and sometimes numbers to define the id variable.

As far as this goes:

describe some of the usages you have developed from this change

Details will have to wait a bit, since I'm still working out some problems with how mutations work in our implementation. In short, we want to be able to create non-GQL functions that can operate directly on the store through a local schema, but co-operate and extend real GQL endpoints through schema-stiching. This way we can gradually migrate our app to GQL, while adding Observable GQL querying for data fetched by other, legacy means (like REST).

@kamilkisiela glad it's working out for you! You can probably re-use my trick for testing your implementation by running the apollo-cache-inmemory's tests themselves, only with your own Cache:

apollo-client/packages/apollo-cache-inmemory/src/__tests__/mapCache.ts

Lines 1 to 20 in 07c9609

	jest.mock('../objectCache', () => {
	const { MapCache, mapNormalizedCacheFactory } = require('../mapCache');
	return {
	ObjectCache: MapCache,
	defaultNormalizedCacheFactory: mapNormalizedCacheFactory,
	};
	});
	describe('MapCache', () => {
	// simply re-runs all the tests
	// with the alternative implementation of the cache
	require('./objectCache');
	require('./cache');
	require('./diffAgainstStore');
	require('./fragmentMatcher');
	require('./readFromStore');
	require('./diffAgainstStore');
	require('./roundtrip');
	require('./writeToStore');
	});

You'll probably run into the same issue with string/number dataId as written above.

[jbaxleyiii]

This makes me so happy

[jbaxleyiii]

I think we could also just directly cast this instead of type check here. The previous cache impl always cast to a string because the id was assumed a global id or combination of __typename and the id cast to a string.

So I think we could just do ${id} instead of doing the check / cast here.

I'm happy to make that change and push it up either before or after this merge!

[niieani]

I'm not sure we can do ${id} or String(id). In testing, I saw that the dataId is sometimes also a Symbol. This also causes issues when running the tests in jest v21, since the expect().toEqual() has changed the behavior in v21 and started to compare Symbols too. Tests don't account for that, so they break these some cases...

[jbaxleyiii]

@niieani this is an excellent PR! I'm going to pull the trigger and merge it in for a 2.1 release! 🎉

Would you be open to writing an article about this or the work you are doing with Apollo for the Apollo Blog? We would love to feature your contributions and use case.

@kamilkisiela the same goes for all of the new incredible Angular work you are doing! I'm always so thrilled to see Angular and Apollo working well together!

[niieani]

@jbaxleyiii Thanks for merging! The work here was done as a part of a project we're working on at Base Lab, and we're planning to write something for our dev blog and open source its parts. However, I'll ask our publishing team, they might be open to cross-posting for better exposure :)