WIP: Caching #9439
WIP: Caching #9439
Conversation
|
Summary of ChangesHello @maneesht, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust caching system for Data Connect queries, enabling client-side storage of query results to improve performance and reduce network requests. It involves significant architectural changes to the query management system, updates to the public API for cache configuration, and expands the underlying GraphQL schema definitions to support these new capabilities. The changes aim to provide developers with more control over data fetching strategies and optimize application responsiveness. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let curObj: Record<string | number, unknown> = toUpdate; | ||
| for (const slice of path) { | ||
| if (typeof curObj[slice] !== 'object') { | ||
| curObj[slice] = {}; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
![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 to the Data Connect SDK, which is a significant and valuable addition. The changes are extensive, touching API definitions, core logic, and test infrastructure. While the overall direction is excellent, I've identified a few areas for improvement. Specifically, there's a potentially confusing API name (PersistentStub for an in-memory cache), a bug in the option handling logic within register.ts, and some issues in a new build script and generated documentation. Addressing these points will enhance the clarity and robustness of the new caching feature.
| newOpts = { | ||
| ...JSON.parse(connectorConfigStr), | ||
| ...newOpts | ||
| }; |
There was a problem hiding this comment.
The order of spreading options here is inconsistent with how they are spread in getDataConnect.ts. Here, newOpts (which contains realSettings) overwrites properties from connectorConfigStr, effectively resulting in { ...sortedSerialized, ...realSettings }. In getDataConnect, the order is { ...realSettings, ...sortedSerialized }. This inconsistency can lead to subtle bugs depending on how the DataConnect instance is created. To ensure consistent behavior, the spread order should be the same in both places.
| newOpts = { | |
| ...JSON.parse(connectorConfigStr), | |
| ...newOpts | |
| }; | |
| newOpts = { | |
| ...newOpts, | |
| ...JSON.parse(connectorConfigStr) | |
| }; |
| export class PersistentStub { | ||
| // (undocumented) | ||
| type: 'MEMORY'; |
There was a problem hiding this comment.
The name PersistentStub is misleading for a cache provider with type: 'MEMORY'. This is caused by an alias for MemoryStub in api.browser.ts. To avoid confusion for developers, it would be clearer to use a name that accurately reflects its in-memory nature, such as MemoryStub.
e2e/data-connect/dataconnect-generated/js/default-connector/.guides/usage.md
Show resolved
Hide resolved
package.sh
Outdated
| #!/bin/bash | ||
| cd packages/firebase | ||
| yarn build | ||
| npm pkg | ||
| cp packages/firebase/firebase-12.3.0.tgz No newline at end of file |
There was a problem hiding this comment.
This script has a few issues that could be improved for robustness:
npm pkgon line 4 is likely a typo fornpm pack, which is used to create a package tarball.npm pkgis for managingpackage.json.- The
cpcommand on line 5 is missing a destination, making it a no-op. It should specify where to copy the created tarball. - The version
12.3.0is hardcoded. This will break when the version changes. It would be more robust to extract the version frompackage.json.
| #!/bin/bash | |
| cd packages/firebase | |
| yarn build | |
| npm pkg | |
| cp packages/firebase/firebase-12.3.0.tgz | |
| #!/bin/bash | |
| cd packages/firebase | |
| yarn build | |
| npm pack | |
| VERSION=$(node -p "require('./package.json').version") | |
| cp "firebase-$VERSION.tgz" ../../ |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (253,967 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
2 participants
No description provided.