👍 Automatically wait the target plugin on denops.dispatch()
#340
Conversation
WalkthroughThe changes bring enhancements to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- denops/@denops-private/denops.ts (3 hunks)
- denops/@denops-private/service.ts (3 hunks)
Additional comments not posted (4)
denops/@denops-private/denops.ts (2)
20-20: The addition of theloadedmethod to theServicetype aligns with the PR's objective to handle plugin load states effectively.
79-92: The updateddispatchmethod now correctly handles the scenario where a plugin might not be loaded at the time of the dispatch call. This change is crucial for preventing race conditions and ensuring that plugin dependencies are managed correctly.denops/@denops-private/service.ts (2)
63-65: The implementation of theloadedmethod in theServiceclass is straightforward and correctly checks if a plugin is loaded by querying the internal plugins map. This method is essential for the new dispatch logic indenops.ts.
133-162: The addition of custom events (denops:pluginloadstart,denops:pluginloadend,denops:pluginloadfail) enhances the transparency of the plugin loading process. These events provide valuable debugging information and help in managing plugin dependencies more effectively.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- denops/@denops-private/denops.ts (3 hunks)
- denops/@denops-private/denops_test.ts (3 hunks)
- denops/@denops-private/service.ts (3 hunks)
- denops/@denops-private/service_test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- denops/@denops-private/denops.ts
- denops/@denops-private/service.ts
Additional comments not posted (4)
denops/@denops-private/denops_test.ts (3)
25-25: The addition of theloadedmethod to theServicemock aligns with the changes in theServiceclass to support checking if a plugin is loaded. This is crucial for the new functionality ensuring plugins are loaded before dispatching.
99-111: The updated test fordispatch()correctly integrates theloadedmethod to ensure the plugin is loaded before dispatching. This is a critical update for the new functionality and is well implemented here.
115-146: This new test scenario fordispatch()effectively demonstrates the waiting mechanism until the plugin is loaded before proceeding with the dispatch. The use ofdelayanddeadlineto simulate asynchronous behavior and the dispatching of a custom event to signal plugin load completion are well implemented. This test is crucial for verifying the new functionality.denops/@denops-private/service_test.ts (1)
35-36: The new test scenario for theload()method correctly handles the case where no host is bound to the service, ensuring robust error handling. This is an important test for the reliability of the service operations.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
==========================================
+ Coverage 89.81% 90.01% +0.20%
==========================================
Files 9 9
Lines 648 661 +13
Branches 61 62 +1
==========================================
+ Hits 582 595 +13
Misses 66 66 ☔ View full report in Codecov by Sentry. |
|
dispatch fails if plugin registered and populate dispatcher at test with
import type { Denops } from "https://deno.land/x/denops_std@v6.0.1/mod.ts";
export async function main(denops: Denops) {
await new Promise((resolve) => setTimeout(resolve, 100));
await denops.dispatch("test_b", "hello");
}
import type { Denops } from "https://deno.land/x/denops_std@v6.0.1/mod.ts";
export async function main(denops: Denops) {
await new Promise((resolve) => setTimeout(resolve, 200));
denops.dispatcher = {
hello() {
console.log("hello");
},
};
await denops.dispatch("test_b", "hello");
} |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
denops/@denops-private/denops_test.ts (1)
2-2: Ensure consistent use of imports.Consider organizing imports by grouping them from the same module together for better readability.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- denops/@denops-private/denops.ts (2 hunks)
- denops/@denops-private/denops_test.ts (3 hunks)
- denops/@denops-private/service.ts (3 hunks)
- denops/@denops-private/service_test.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- denops/@denops-private/denops.ts
- denops/@denops-private/service_test.ts
Additional comments not posted (8)
denops/@denops-private/denops_test.ts (4)
8-8: Good addition ofpromiseStatefor asynchronous state checks.
25-25: Correct implementation ofwaitLoadedas a stub.
99-111: Properly implemented stubbing forwaitLoadedanddispatchin thedispatch()test.
116-135: Excellent test to ensuredispatch()waits forwaitLoaded()before proceeding.This test effectively captures the asynchronous behavior introduced in the
dispatch()method, ensuring that thewaitLoadedmethod is called and completed beforedispatchis executed.denops/@denops-private/service.ts (4)
14-14: Proper initialization of#waitersto manage asynchronous plugin loading.
22-27: Correct implementation of#getWaiterto ensure a promise is created if not already present.
Line range hint
33-52: Ensure error handling is robust inloadmethod.This method should robustly handle errors, especially since it deals with loading plugins which can fail for various reasons. The current implementation throws an error if no host is bound, but it should also handle other potential issues like script loading failures.
Verification successful
The verification of the
loadmethod in theServiceclass within thedenops/@denops-private/service.tsfile has revealed that there are multiple instances where errors are explicitly thrown to handle potential issues:
- An error is thrown if no host is bound to the service.
- An error is thrown if a requested plugin is not loaded.
These checks ensure that the method handles some critical error scenarios robustly, particularly those related to the absence of necessary components like the host and the plugin. This aligns with the requirement for robust error handling in a method that deals with loading plugins, which can indeed fail for various reasons as noted in the review comment.
- File: denops/@denops-private/service.ts
- Error thrown if no host is bound: Ensures that the service cannot operate without a host.
- Error thrown if a plugin is not loaded: Prevents operations on undefined or uninitiated plugins.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in the `load` method of the `Service` class. # Test: Search for error handling patterns in the `load` method. Expect: Proper error handling for plugin loading. rg --type typescript 'throw new Error' 'denops/@denops-private/service.ts'Length of output: 109
Script:
#!/bin/bash # Description: Verify error handling in the `load` method of the `Service` class. # Test: Search for error handling patterns in the `load` method. Expect: Proper error handling for plugin loading. rg 'throw new Error' denops/@denops-private/service.tsLength of output: 228
73-75: Correct implementation ofwaitLoadedto wait for a plugin to be fully loaded.
Note that wait only happens on `denops.dispatch()` but `service.dispatch()` so this change only affects plugin-plugin communication (not denops-plugin communication)
|
Followed #340 (comment), most of code is re-written from scratch so I rebased & force pushed. |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
denops/@denops-private/service.ts (3)
Line range hint
39-58: Consider replacingconsole.logwith a more robust logging mechanism.Using
console.logfor debugging is not ideal in production environments. Consider integrating a more robust logging framework that can be configured for different environments.
Line range hint
91-117: Enhance error handling in asynchronous dispatch.Consider adding more detailed error logging and possibly retries or fallback mechanisms in the
dispatchAsyncmethod to handle failures more gracefully.
Line range hint
160-166: Include more error details in the serialized error format.Consider including more details about the error in the
toVimErrorfunction to aid in debugging when errors are passed to Vim/Neovim.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- denops/@denops-private/denops.ts (2 hunks)
- denops/@denops-private/denops_test.ts (3 hunks)
- denops/@denops-private/service.ts (3 hunks)
- denops/@denops-private/service_test.ts (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- denops/@denops-private/denops.ts
- denops/@denops-private/denops_test.ts
- denops/@denops-private/service_test.ts
Additional comments not posted (3)
denops/@denops-private/service.ts (3)
9-13: Define aWaitertype to handle promises with resolvers.This is a good approach to manage asynchronous operations where you need to manually resolve promises.
28-33: Ensure proper management of asynchronous plugin loading.The
#getWaitermethod correctly manages the creation and retrieval of waiters for plugins, which is essential for the new asynchronous loading feature.
79-81: Proper implementation of waiting for plugin loading.The
waitLoadedmethod correctly returns the promise associated with the waiter for the plugin, ensuring that the caller can await the plugin's loading process.
Now, denops.dispatch() automatically waits for the target plugin to load, enabling child plugins (e.g., Shougo/ddu-ui-filter for Shougo/ddu) to call the parent plugin's API at any time.
Summary by CodeRabbit
New Features
Servicetype to includewaitLoadedin addition todispatch.waitLoaded()to theserviceobject.Bug Fixes
assertThrowswithassertRejectsin theload()method test for better error handling.Documentation