-
Notifications
You must be signed in to change notification settings - Fork 7
Log sink api #83
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
Log sink api #83
Conversation
…ift-Enterprise to version 3.3.0
*SDK version migrated from 3.2.1 to 3.3 in both Android and iOS. *All the submodules are now pointing to the main branch. *Create an error for all unimplemented functions since we are not releasing p2p in v1.0.0
- Add collection change listener tests - Add document change listener tests - Add custom bug fix tests - Update CblReactnativeModule.kt for Android - Update CblReactNativeEngine.tsx - Update iOS Swift bridge code - Update test navigation sections - Update cblite-js tests
- Add collection-change-listener test component - Add custom-bug-fix test component - Add document-change-listener test component - Update CblReactNativeEngine with latest changes - Update Android module with latest changes - Resolve merge conflicts in Podfile.lock
- Implement collection_GetFullName method in CblReactnativeModule for both Android and iOS. - Update CblReactNativeEngine to include collection_GetFullName functionality. - Modify various components to utilize the new fullName method for collections. - Expand tsconfig.build.json to exclude additional directories.
- Add listenerToken_Remove method to CblReactNativeEngine - Update Android native module with listener token removal - Update iOS native module with listener token removal - Update listener test files to use new token-based removal - Add listener token plan documentation
- Update Android CblReactnativeModule.kt - Update iOS CblReactnative.swift - Remove listener-token-plan.md (no longer needed)
New remove listener api
chore: update submodule reference for cblite-js-tests
…d ios cbl-js-swift
chore: update submodules to support_col_db_property branch
…d ios cbl-js-swift
Summary of ChangesHello @jayant-dhingra-cb, 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 significantly enhances the logging capabilities of Couchbase Lite React Native by introducing a new Log Sink API. Developers can now configure console, file, and custom log destinations with specified levels and domains. Alongside this, the PR includes a major refactoring of how change listeners are managed across the native modules, moving to a unified token-based system for improved consistency and ease of use. The underlying Couchbase Lite SDKs have also been updated to version 3.3.0, and new examples and tests are provided to cover these changes. Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new Log Sinks API and significantly refactors the change listener management across both Android and iOS platforms. The listener refactoring, which unifies different listener types under a single management system, is a great improvement for code clarity and maintainability. The new Log Sinks API is well-implemented with corresponding test files. I've identified a few areas for improvement, including replacing GlobalScope with a lifecycle-aware CoroutineScope on Android for better resource management, fixing some minor import formatting in test files, and correcting a duplicate key issue in the test navigation configuration.
| id: 15, | ||
| title: 'LogSinks API', | ||
| path: '/tests/logsinks', | ||
| }, | ||
| { | ||
| id: 16, | ||
| title: 'Log Sink Console Logs', | ||
| path: '/tests/console-logging', | ||
| }, | ||
| { | ||
| id: 17, | ||
| title: 'File Logging Test', | ||
| path: '/tests/file-logging', | ||
| }, | ||
| { | ||
| id: 18, | ||
| title: 'Custom Logging Test', | ||
| path: '/tests/custom-logging', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are duplicate id values in this array, starting with id: 15 on this line. React keys for list items must be unique to avoid potential rendering issues. I've suggested renumbering the subsequent items to ensure all IDs are unique.
| id: 15, | |
| title: 'LogSinks API', | |
| path: '/tests/logsinks', | |
| }, | |
| { | |
| id: 16, | |
| title: 'Log Sink Console Logs', | |
| path: '/tests/console-logging', | |
| }, | |
| { | |
| id: 17, | |
| title: 'File Logging Test', | |
| path: '/tests/file-logging', | |
| }, | |
| { | |
| id: 18, | |
| title: 'Custom Logging Test', | |
| path: '/tests/custom-logging', | |
| id: 16, | |
| title: 'LogSinks API', | |
| path: '/tests/logsinks', | |
| }, | |
| { | |
| id: 17, | |
| title: 'Log Sink Console Logs', | |
| path: '/tests/console-logging', | |
| }, | |
| { | |
| id: 18, | |
| title: 'File Logging Test', | |
| path: '/tests/file-logging', | |
| }, | |
| { | |
| id: 19, | |
| title: 'Custom Logging Test', | |
| path: '/tests/custom-logging', |
| scopeName: String, | ||
| promise: Promise | ||
| ) { | ||
| GlobalScope.launch(Dispatchers.IO) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using GlobalScope is discouraged as it can lead to memory leaks and makes coroutines difficult to manage and test. It's better to use a CoroutineScope that is tied to the lifecycle of the module. You can create a CoroutineScope at the class level and cancel it in onCatalystInstanceDestroy(). This change should be applied to all usages of GlobalScope.launch in this file (lines 1641, 1668, 1696).
Example of creating a module-scoped coroutine:
class CblReactnativeModule(...) {
private val moduleScope = CoroutineScope(SupervisorJob() + Dispatchers.Default)
override fun onCatalystInstanceDestroy() {
super.onCatalystInstanceDestroy()
moduleScope.cancel()
}
// ... in your method
@ReactMethod
fun collection_GetFullName(...) {
moduleScope.launch(Dispatchers.IO) {
// ...
}
}
}| MutableDocument, | ||
| Document, | ||
| ConcurrencyControl | ||
| } from 'cbl-reactnative';import getFileDefaultPath from '@/service/file/getFileDefaultPath'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a formatting issue here. The import statement for getFileDefaultPath should be on a new line for better readability and to follow standard code style.
| } from 'cbl-reactnative';import getFileDefaultPath from '@/service/file/getFileDefaultPath'; | |
| } from 'cbl-reactnative'; | |
| import getFileDefaultPath from '@/service/file/getFileDefaultPath'; |
| MutableDocument, | ||
| Document, | ||
| ConcurrencyControl | ||
| } from 'cbl-reactnative';import getFileDefaultPath from '@/service/file/getFileDefaultPath'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a formatting issue here. The import statement for getFileDefaultPath should be on a new line for better readability and to follow standard code style.
| } from 'cbl-reactnative';import getFileDefaultPath from '@/service/file/getFileDefaultPath'; | |
| } from 'cbl-reactnative'; | |
| import getFileDefaultPath from '@/service/file/getFileDefaultPath'; |
| Document, | ||
| ConcurrencyControl, | ||
| ListenerToken | ||
| } from 'cbl-reactnative';import getFileDefaultPath from '@/service/file/getFileDefaultPath'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a formatting issue here. The import statement for getFileDefaultPath should be on a new line for better readability and to follow standard code style.
| } from 'cbl-reactnative';import getFileDefaultPath from '@/service/file/getFileDefaultPath'; | |
| } from 'cbl-reactnative'; | |
| import getFileDefaultPath from '@/service/file/getFileDefaultPath'; |
No description provided.