Make KvStore.list() method required instead of optional
#506
Conversation
This is a breaking change for Fedify 2.0.0. The list() method was added as optional in 1.10.0 to give existing implementations time to add support. All official KvStore implementations already support this method. Changes: - Update KvStore interface to make list() required - Add list() to MockKvStore and test mocks - Simplify FedifySpanExporter by removing cas()-only fallback path - Remove non-null assertions from all list() calls in tests - Update documentation to reflect the change Closes fedify-dev#499 Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @dahlia, 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 formalizes the 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
|
![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 makes the KvStore.list() method required, which is a significant step for Fedify 2.0.0. The changes are well-executed across the codebase, including updating the KvStore interface, removing fallback logic that relied on the optional nature of the method, and updating tests to remove non-null assertions. The documentation has also been updated to reflect this change. I've suggested a minor wording improvement in the documentation to make the requirement clearer for developers implementing the KvStore interface. Overall, this is a solid pull request.
There was a problem hiding this comment.
Pull request overview
This PR makes the KvStore.list() method required in Fedify 2.0.0, completing the transition that began in version 1.10.0 when the method was introduced as optional. The change simplifies the codebase by removing conditional logic and allowing all code to rely on the list() method being present.
- Removes optional marker from
KvStore.list()method signature - Updates all test files to remove non-null assertion operators (
!) - Adds
list()implementation to mock KvStore instances used in tests - Simplifies FedifySpanExporter by removing fallback logic for stores without
list()
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/fedify/src/federation/kv.ts |
Updates interface to make list() required instead of optional |
packages/fedify/src/utils/kv-cache.ts |
Adds list() implementation to MockKvStore |
packages/fedify/src/utils/kv-cache.test.ts |
Adds list() to test KvStore implementation |
packages/fedify/src/otel/exporter.ts |
Removes conditional logic for optional list(), simplifies implementation |
packages/fedify/src/otel/exporter.test.ts |
Updates tests to remove cas-only test case, adds list() to custom KvStore |
packages/sqlite/src/kv.test.ts |
Removes non-null assertion operators from list() calls |
packages/redis/src/kv.test.ts |
Removes non-null assertion operators from list() calls |
packages/postgres/src/kv.test.ts |
Removes non-null assertion operators from list() calls |
packages/fedify/src/federation/kv.test.ts |
Removes non-null assertion operators from list() calls |
packages/denokv/src/mod.test.ts |
Removes non-null assertion operators from list() calls |
packages/cfworkers/test/kv.test.ts |
Removes non-null assertion operators from list() calls |
docs/manual/kv.md |
Updates documentation to reflect list() as required method |
CHANGES.md |
Adds changelog entry for breaking change |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Add required list() method stubs to MyCustomKvStore examples in the KvStore documentation to satisfy the KvStore interface requirements. Also increase Node.js memory limit for docs build to prevent OOM errors. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
This makes the
KvStore.list()method required in Fedify 2.0.0.The method was added as optional in 1.10.0 (#498, #500) to give existing implementations time to add support. Since all official
KvStoreimplementations already support this method, it is now required.See CHANGES.md and docs/manual/kv.md for details.
Closes #499.