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

Trigger watches when write transactions update cache. #187

Conversation

rhishikeshj
Copy link
Contributor

@rhishikeshj rhishikeshj commented Nov 30, 2017

Context
If the cache is updated from an read-write transaction, watch queries are not triggered even though the data is updated.

Solution
Added a new member to the ReadWriteTransaction object to get the ApolloStore object.
Triggered the subscriber.store functions to make sure watch queries are triggered.

This is required because the subscribers and subscriber.store function both are dependent upon the current ApolloStore object on which the transaction is executing.
If this breaks the abstraction of the ReadWriteTransaction in any way, let me know and also if you could suggest a better way to do this.

@apollo-cla
Copy link

@rhishikeshj: 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/

@martijnwalraven
Copy link
Contributor

Thanks, this looks great! Happy you included a test.

Maybe it would be cleaner to extract subscriber notification from publish() into its own fileprivate didChangeKeys(_ changedKeys:context:) function on ApolloStore. That avoids code duplication and respects encapsulation a little better. We could then pass that function into the transaction (as self. didChangeKeys) instead of the store itself.

@rhishikeshj
Copy link
Contributor Author

@martijnwalraven Is this closer to what you would be comfortable with ?

@martijnwalraven martijnwalraven merged commit a5a3e1d into apollographql:master Dec 5, 2017
@martijnwalraven
Copy link
Contributor

Looks good to me, thanks!

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.

3 participants