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

Defer: take @defer cases into account with normalized cache #3987

Merged
merged 4 commits into from
Apr 12, 2022

Conversation

BoD
Copy link
Contributor

@BoD BoD commented Apr 1, 2022

With defer, multiple elements can be emitted in Flows returned by the network, which prompted a few adaptations in the normalized cache interceptors.

Also, always read deferred fragments in the adapter when used to parse cached values.

@netlify
Copy link

netlify bot commented Apr 1, 2022

Deploy Preview for apollo-android-docs canceled.

Name Link
🔨 Latest commit 5ae4d7d
🔍 Latest deploy log https://app.netlify.com/sites/apollo-android-docs/deploys/624ef984cd49400009b8570b

@BoD BoD marked this pull request as ready for review April 1, 2022 17:20
@BoD BoD requested review from sav007 and martinbonnin as code owners April 1, 2022 17:20
@@ -72,26 +74,25 @@ val CacheFirstInterceptor = object : ApolloInterceptor {
} else {
throw it
}
}.singleOrNull()
}.map { response ->
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the contract that CacheFirst emits only once. I guess there's no real way around this but the doc needs to be updated

@BoD BoD force-pushed the defer-normalized-cache-interceptor-updates branch from e2caa01 to 5ae4d7d Compare April 7, 2022 14:47
"""{"data":{"computers":[{"__typename":"Computer","id":"Computer1"}]},"hasNext":true}""",
"""{"data":{"cpu":"386","year":1993,"screen":{"__typename":"Screen","resolution":"640x480"}},"path":["computers",0],"hasNext":true}""",
"""{"data":{"isColor":false},"path":["computers",0,"screen"],"hasNext":false,"label":"a"}""",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

(Dog) Food for thought: looks like we could use test builders to simplify those tests

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

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

LGTM !
2 potential follow ups:

  • Use test builders in Tests
  • Stop throwing in the flow instead expose Response.exception

@BoD BoD merged commit 1855851 into main Apr 12, 2022
@BoD BoD deleted the defer-normalized-cache-interceptor-updates branch April 12, 2022 07:41
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.

2 participants