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 clear cache from the latest Apollo #141

Merged
merged 4 commits into from
Jan 11, 2019

Conversation

larryonoff
Copy link
Contributor

@larryonoff larryonoff commented Jan 3, 2019

Issue: #36

Description of changes:
Added functionality to clear client cache manually.

The was just copied from the latest Apollo + AWSSQLLiteNormalizedCache conformance of the new clear() method.

Hope that taking the only clear cache from #131 will speed up the acceptance of this PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@palpatim palpatim left a comment

Choose a reason for hiding this comment

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

  1. Let's add a protocol (or set of protocols) like AWSAppSyncApolloExtensions to clearly define the changes we're adding to the core Apollo. That will help us identify the changes we've made when the time comes to upgrade.
  2. The change on the face of it looks good, but we will need to have test coverage of the new functions in order to merge. We can do this when we evaluate & merge the PR, but having tests as part of this PR would speed up the process.

@larryonoff
Copy link
Contributor Author

@palpatim this PR includes copy of the functionality from Apollo. So I don't think that it should be included into AWSAppSyncApolloExtensions.

@larryonoff
Copy link
Contributor Author

Just added test

@palpatim
Copy link
Contributor

palpatim commented Jan 7, 2019

@palpatim this PR includes copy of the functionality from Apollo. So I don't think that it should be included into AWSAppSyncApolloExtensions.

I don't feel super strongly the protocol as the only way to document the changes we're making from the core Apollo code, but I do want to make sure we capture the variations that we introduce so we can ease the transition for the future. What about a separate /AWSAppSyncClient/Apollo/README.md file with a section that specifically calls out this change. That file would then be used to help us audit changes that may arise from a future Apollo upgrade.

(I recognize that this in some sense closes the barn door after the horse has escaped--we've already made changes to Apollo that haven't been clearly documented--so it's on us to go back and complete that documentation as I described in my comment on #131.)

@larryonoff
Copy link
Contributor Author

@palpatim I complete agree with you idea. But as I mentioned this PR doesn't have any specific changes, it copies 1-by-1 code from the latest Apollo repository.

I propose the following approach.

  1. Merge the latest Apollo into AWSAppSync repo. This can be difficult now, since Apollo changed GraphQL protocols / classes model. So that Amplify CLI codegen should be updated. BUT I'm working on AppSync document that has suggestion to introduce templates for code generation. Very similar to
    SwiftGen, but e.g. based on [mustache.js](url
    https://github.com/janl/mustache.js/).
  2. Document as you suggested /AWSAppSyncClient/Apollo/README.md all specific changes that Amazon team did and keep them in a separate file. I just think that document should be in different folder, e.g. /AWSApolloExtensions.md.
  3. Try don't touch Apollo code directly.
  4. Create PR for main Apollo repository. And as soon they accept these changes, merge them into AppSync.

@palpatim
Copy link
Contributor

  1. Merge the latest Apollo into AWSAppSync repo.

As another part of this, we need to ensure we have AppSync-specific tests that cover the behavior we're relying on, especially the behavior we changed (closely related to the documentation requirement below).

This can be difficult now, since Apollo changed GraphQL protocols / classes model. So that Amplify CLI codegen should be updated. BUT I'm working on AppSync document that has suggestion to introduce templates for code generation. Very similar to SwiftGen, but e.g. based on [mustache.js)(https://github.com/janl/mustache.js/).

I look forward to seeing a proposal. As I'm sure you recognize, this would be a pretty large undertaking, since it needs to be something we can support cross-platform.

  1. Document as you suggested /AWSAppSyncClient/Apollo/README.md all specific changes that Amazon team did and keep them in a separate file. I just think that document should be in different folder, e.g. /AWSApolloExtensions.md.

The reason I suggested a README at the top level of the Apollo directory is that it slightly increases discoverability for people who browse to the Apollo directory on GitHub.

However, it's probably worth calling out in the top-level README as well, probably in an "AWSAppSync and Apollo" section or similar. I'll take that change on for a followup to this PR.

  1. Try don't touch Apollo code directly.
  2. Create PR for main Apollo repository. And as soon they accept these changes, merge them into AppSync.

I think the Apollo-specific pieces you've outlined here are a good goal, but obviously getting PRs accepted into Apollo would be outside our control. :)

@palpatim palpatim merged commit 1aa5ac9 into awslabs:master Jan 11, 2019
@larryonoff larryonoff deleted the clear-cache branch January 15, 2019 18:02
@larryonoff
Copy link
Contributor Author

I look forward to seeing a proposal. As I'm sure you recognize, this would be a pretty large undertaking, since it needs to be something we can support cross-platform.

I think this's a huge par of work. I won't be able doing it since I don't have so many time on open-source to do it. So I just can cross fingers that someone implements it.

I think the Apollo-specific pieces you've outlined here are a good goal, but obviously getting PRs accepted into Apollo would be outside our control. :)

I agree, but it depends. But here's simple rule: if PR is accepted then just merge latest Apollo, if not - leave it as an internal AppSync feature.

@palpatim
Copy link
Contributor

Released on 2.9.2

@airstance
Copy link

Just an FYI in case someone else stumbles on this... This call only deletes the data cache. It does not touch the other new caches for AppSync (subscriptions, offline writes)

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

Successfully merging this pull request may close these issues.

3 participants