Conversation
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
comments resolved - LGTM (waiting to approve until other comments resolve)
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
all comments resolved - LGTM
|
LGTM pending the comments. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive caching layer for Data Connect, which is a significant and well-executed feature. The architecture is robust, handling entity normalization, cache invalidation on user changes, and providing different fetch policies. The code is well-structured with good separation of concerns and is accompanied by extensive tests. I've identified one high-severity issue with the implementation of the CACHE_ONLY fetch policy, where it incorrectly falls back to a network request. A code suggestion to fix this is provided. Overall, this is an excellent contribution.
hsubox76
left a comment
hsubox76
left a comment
There was a problem hiding this comment.
Add a changeset. Looks like a new feature so probably "minor" and make sure to add 'firebase': minor also.
Changeset File Check
|
hsubox76
left a comment
hsubox76
left a comment
There was a problem hiding this comment.
Looks like yarn format needs to be run also.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@firebase/data-connect": minor | |||
There was a problem hiding this comment.
Add "firebase": minor and "@firebase/util": minor
11 participants
No description provided.