Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add synchronous version of persistCache #299

Merged
merged 12 commits into from
Oct 23, 2019

Conversation

andrewshirk
Copy link

This PR is intended to provide a simple, non-breaking way to start persisting a cache by first restoring the cache synchronously, for apps with small localStorage requirements. This is just a proposal at this point.

@apollo-cla
Copy link

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

@codecov-io
Copy link

codecov-io commented Oct 16, 2019

Codecov Report

Merging #299 into master will increase coverage by 2.15%.
The diff coverage is 92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
+ Coverage   72.95%   75.11%   +2.15%     
==========================================
  Files          10       11       +1     
  Lines         196      221      +25     
  Branches       30       33       +3     
==========================================
+ Hits          143      166      +23     
- Misses         50       52       +2     
  Partials        3        3
Impacted Files Coverage Δ
src/index.ts 100% <100%> (ø) ⬆️
src/persistCacheSync.ts 91.66% <91.66%> (ø)

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 3d2bb9c...eb7cab0. Read the comment docs.

Copy link
Collaborator

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome change! Thank you so much for controbution. We are missing following items to get it merged:

  • Readme.md chapter on sync api
  • Unit test - if can be simply copy paste with changed classes
  • exporting file to package level so it can be used

Once again thank you so much for this awesome contribution.

PS: I'm traveling for the rest of the week so I will be able to merge it on next monday after testing it and release the change.

src/syncPersistCache.js Outdated Show resolved Hide resolved
src/syncPersistCache.js Outdated Show resolved Hide resolved
src/syncPersistCache.js Outdated Show resolved Hide resolved
src/syncPersistCache.js Outdated Show resolved Hide resolved
@wtrocki
Copy link
Collaborator

wtrocki commented Oct 16, 2019

Some minor changes suggested.

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 17, 2019

Awesome work. I will sit down next week and make sure that this is merged and new version of package is released.

@andrewshirk
Copy link
Author

Thanks for your support on this!

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 21, 2019

Reviewing!

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 21, 2019

@andrewshirk I see that unit tests are not passing. I see some issues when playing with them locally. Are you still working on this? I do not want to duplicate work.
Apart from that, this is an solid merge - I would also get that released by official channel and promote this change on twitter.

@andrewshirk
Copy link
Author

@wtrocki I haven't done any more work on it yet. I ran out of time last week, but will probably have some tonight. Just let me know what'd you like. Thanks again!

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 21, 2019

Generally I see that tests using simulateApp that still uses async. Generally I would simply write separate unit tests that tests only restore. This should not be hard and it will test all code that we have added. Repeating all tests for the sync api makes no sense - we just want to check addition we have made. I would just remove current tests and write simple one that will just test async api.
There is no need for mocks as well.

Thank you so much for contribution.

@andrewshirk
Copy link
Author

@wtrocki Great, thanks for the clarification. You suggested copy/pasting earlier, so wasn't sure about that. I will get to it ASAP.

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 21, 2019

It was my mistake for not being clear. I should have said to copy paste only the restore test and ignore the rest.

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 22, 2019

Very simple test that works for me:

import { InMemoryCache } from 'apollo-cache-inmemory';
import { persistCacheSync, SynchronousCachePersistor } from '../';
import MockStorage from '../__mocks__/MockStorage';
import { ApolloLink } from 'apollo-link';
import Observable = require('zen-observable');
import ApolloClient from 'apollo-client';
import gql from 'graphql-tag';

jest.useFakeTimers();
describe('persistCacheSync', () => {
  describe('setup', () => {
    it('persist cache', async () => {
      const operation = gql`
      {
        hello
      }
    `;
    const result = { data: { hello: 'world' } };
      const storage = new MockStorage();
      const cache = new InMemoryCache();

      const cachePersistor = new SynchronousCachePersistor({ cache, storage });

      const link = new ApolloLink(() => Observable.of(result));
      const client = new ApolloClient({ cache, link });
      expect(cache.extract()).toEqual({});

      await client.query({ query: operation });
      jest.runTimersToTime(
        persistOptions.debounce ? persistOptions.debounce + 1 : 1001
      );

      const cache2 = new InMemoryCache();
      const cachePersistor2 = new SynchronousCachePersistor({ cache: cache2, storage });
      cachePersistor2.restoreSync();
      const keys = Object.keys(cache2.extract());
      expect(keys.length).toEqual(2);
    });
  });
});

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 22, 2019

Sadly I cannot push changes to your branch, but I have also needed to change:
export * from './persistCacheSync'; in index.ts
and exported every sync version (users will not be able to use it without exporting them)

@andrewshirk
Copy link
Author

Wonderful. I'll have time to work on this tonight.

@andrewshirk
Copy link
Author

@wtrocki I'll investigate the test failures.

@andrewshirk
Copy link
Author

MockStorage is an async API, so we need a MockStorageSync I suppose.

@andrewshirk
Copy link
Author

@wtrocki I gave you push access to my repo, if you want to make any final tweaks.

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 23, 2019

Awesome work. I will play with it in sample app and merge changes. Thank you so much

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 23, 2019

@andrewshirk Amazing collaboration and great work on the feature. I have spent some time playing with the new API. Merging the change and doing a new release of dev package.

@wtrocki wtrocki merged commit 079bc09 into apollographql:master Oct 23, 2019
@andrewshirk
Copy link
Author

@wtrocki Super! Thanks again for all your help and support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants