Revert "Revert "Support import_map.json[c] in denops plugin directory""
#450
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Service
participant Plugin
participant Denops
participant PluginScript
Service->>Plugin: new Plugin(denops, name, script)
Plugin->>Plugin: resolve script URL (+suffix)
Plugin->>Plugin: search for import map
Plugin->>PluginScript: dynamic import (with/without import map)
PluginScript-->>Plugin: exports main(denops)
Plugin->>Denops: emit "plugin:pre:load"
Plugin->>PluginScript: call main(denops)
PluginScript-->>Plugin: returns disposable
Plugin->>Denops: emit "plugin:post:load"
Service->>Plugin: call(fn, ...args)
Plugin->>PluginScript: dispatcher[fn](...args)
PluginScript-->>Plugin: result
Service->>Plugin: unload()
Plugin->>Denops: emit "plugin:pre:unload"
Plugin->>PluginScript: dispose()
Plugin->>Denops: emit "plugin:post:unload"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsdenops/@denops-private/plugin.ts (4)Learnt from: Milly Learnt from: lambdalisue Learnt from: Milly Learnt from: Milly 🪛 Biome (2.1.2)denops/@denops-private/plugin.ts[error] 20-20: This variable is used before its declaration. The variable is declared here: (lint/correctness/noInvalidUseBeforeDeclaration) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (7)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
==========================================
- Coverage 96.97% 96.52% -0.45%
==========================================
Files 26 27 +1
Lines 1421 1468 +47
Branches 182 192 +10
==========================================
+ Hits 1378 1417 +39
- Misses 40 47 +7
- Partials 3 4 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Plan: Pro
📒 Files selected for processing (8)
deno.jsonc(1 hunks)denops/@denops-private/plugin.ts(1 hunks)denops/@denops-private/plugin_test.ts(1 hunks)denops/@denops-private/service.ts(1 hunks)tests/denops/testdata/with_import_map/helper.ts(1 hunks)tests/denops/testdata/with_import_map/import_map.json(1 hunks)tests/denops/testdata/with_import_map/import_map.jsonc(1 hunks)tests/denops/testdata/with_import_map/plugin_with_import_map.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Milly
PR: vim-denops/denops.vim#418
File: tests/denops/runtime/functions/plugin/check_type_test.ts:6-6
Timestamp: 2024-09-14T17:09:30.174Z
Learning: In this project, import paths prefixed with `/denops-testdata/` are defined in `deno.jsonc` via an import map, and these import paths are valid.
Learnt from: Milly
PR: vim-denops/denops.vim#385
File: autoload/denops/plugin.vim:56-58
Timestamp: 2024-07-05T15:37:46.867Z
Learning: In the Denops.vim project, the `denops#plugin#unload()` function is designed to throw exceptions to the caller, who is responsible for handling them.
Learnt from: Milly
PR: vim-denops/denops.vim#335
File: denops/@denops-private/service.ts:245-259
Timestamp: 2024-07-27T10:03:47.237Z
Learning: The `emit` function in the Denops plugin system catches and logs errors internally, ensuring it does not throw errors to the caller.
deno.jsonc (1)
Learnt from: Milly
PR: vim-denops/denops.vim#418
File: tests/denops/runtime/functions/plugin/check_type_test.ts:6-6
Timestamp: 2024-09-14T17:09:30.174Z
Learning: In this project, import paths prefixed with `/denops-testdata/` are defined in `deno.jsonc` via an import map, and these import paths are valid.
tests/denops/testdata/with_import_map/import_map.jsonc (1)
Learnt from: Milly
PR: vim-denops/denops.vim#418
File: tests/denops/runtime/functions/plugin/check_type_test.ts:6-6
Timestamp: 2024-09-14T17:09:30.174Z
Learning: In this project, import paths prefixed with `/denops-testdata/` are defined in `deno.jsonc` via an import map, and these import paths are valid.
tests/denops/testdata/with_import_map/import_map.json (1)
Learnt from: Milly
PR: vim-denops/denops.vim#418
File: tests/denops/runtime/functions/plugin/check_type_test.ts:6-6
Timestamp: 2024-09-14T17:09:30.174Z
Learning: In this project, import paths prefixed with `/denops-testdata/` are defined in `deno.jsonc` via an import map, and these import paths are valid.
tests/denops/testdata/with_import_map/helper.ts (1)
Learnt from: Milly
PR: vim-denops/denops.vim#418
File: tests/denops/runtime/functions/plugin/check_type_test.ts:6-6
Timestamp: 2024-09-14T17:09:30.174Z
Learning: In this project, import paths prefixed with `/denops-testdata/` are defined in `deno.jsonc` via an import map, and these import paths are valid.
tests/denops/testdata/with_import_map/plugin_with_import_map.ts (1)
Learnt from: Milly
PR: vim-denops/denops.vim#418
File: tests/denops/runtime/functions/plugin/check_type_test.ts:6-6
Timestamp: 2024-09-14T17:09:30.174Z
Learning: In this project, import paths prefixed with `/denops-testdata/` are defined in `deno.jsonc` via an import map, and these import paths are valid.
denops/@denops-private/plugin.ts (4)
Learnt from: Milly
PR: vim-denops/denops.vim#385
File: autoload/denops/plugin.vim:56-58
Timestamp: 2024-07-05T15:37:46.867Z
Learning: In the Denops.vim project, the `denops#plugin#unload()` function is designed to throw exceptions to the caller, who is responsible for handling them.
Learnt from: Milly
PR: vim-denops/denops.vim#335
File: denops/@denops-private/service.ts:245-259
Timestamp: 2024-07-27T10:03:47.237Z
Learning: The `emit` function in the Denops plugin system catches and logs errors internally, ensuring it does not throw errors to the caller.
Learnt from: lambdalisue
PR: vim-denops/denops.vim#344
File: denops/@denops-private/service.ts:183-183
Timestamp: 2024-07-08T01:52:22.851Z
Learning: In the `denops/@denops-private/service.ts` file, initializing properties like `#loadedWaiter` in the constructor is a common and acceptable pattern.
Learnt from: Milly
PR: vim-denops/denops.vim#418
File: tests/denops/runtime/functions/plugin/check_type_test.ts:6-6
Timestamp: 2024-09-14T17:09:30.174Z
Learning: In this project, import paths prefixed with `/denops-testdata/` are defined in `deno.jsonc` via an import map, and these import paths are valid.
denops/@denops-private/plugin_test.ts (3)
Learnt from: Milly
PR: vim-denops/denops.vim#418
File: tests/denops/runtime/functions/plugin/check_type_test.ts:6-6
Timestamp: 2024-09-14T17:09:30.174Z
Learning: In this project, import paths prefixed with `/denops-testdata/` are defined in `deno.jsonc` via an import map, and these import paths are valid.
Learnt from: Milly
PR: vim-denops/denops.vim#385
File: autoload/denops/plugin.vim:56-58
Timestamp: 2024-07-05T15:37:46.867Z
Learning: In the Denops.vim project, the `denops#plugin#unload()` function is designed to throw exceptions to the caller, who is responsible for handling them.
Learnt from: Milly
PR: vim-denops/denops.vim#335
File: denops/@denops-private/service.ts:245-259
Timestamp: 2024-07-27T10:03:47.237Z
Learning: The `emit` function in the Denops plugin system catches and logs errors internally, ensuring it does not throw errors to the caller.
denops/@denops-private/service.ts (5)
Learnt from: Milly
PR: vim-denops/denops.vim#385
File: autoload/denops/plugin.vim:56-58
Timestamp: 2024-07-05T15:37:46.867Z
Learning: In the Denops.vim project, the `denops#plugin#unload()` function is designed to throw exceptions to the caller, who is responsible for handling them.
Learnt from: lambdalisue
PR: vim-denops/denops.vim#344
File: denops/@denops-private/service.ts:183-183
Timestamp: 2024-07-08T01:52:22.851Z
Learning: In the `denops/@denops-private/service.ts` file, initializing properties like `#loadedWaiter` in the constructor is a common and acceptable pattern.
Learnt from: Milly
PR: vim-denops/denops.vim#352
File: denops/@denops-private/testutil/host.ts:3-4
Timestamp: 2024-07-27T10:03:47.237Z
Learning: `Neovim` and `Vim` from `../host/nvim.ts` and `../host/vim.ts` are classes and should be imported normally if they are instantiated in the code.
Learnt from: Milly
PR: vim-denops/denops.vim#335
File: denops/@denops-private/service.ts:245-259
Timestamp: 2024-07-27T10:03:47.237Z
Learning: The `emit` function in the Denops plugin system catches and logs errors internally, ensuring it does not throw errors to the caller.
Learnt from: Milly
PR: vim-denops/denops.vim#418
File: tests/denops/runtime/functions/plugin/check_type_test.ts:6-6
Timestamp: 2024-09-14T17:09:30.174Z
Learning: In this project, import paths prefixed with `/denops-testdata/` are defined in `deno.jsonc` via an import map, and these import paths are valid.
🧬 Code Graph Analysis (1)
tests/denops/testdata/with_import_map/plugin_with_import_map.ts (1)
tests/denops/testdata/with_import_map/helper.ts (2)
getMessage(3-5)greeting(1-1)
🪛 Biome (1.9.4)
denops/@denops-private/plugin.ts
[error] 20-20: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (26)
tests/denops/testdata/with_import_map/import_map.json (1)
1-7: LGTM! Standard import map structure implemented correctly.The import map follows the standard JSON format with appropriate mappings for test scenarios. The relative path mappings and naming conventions are consistent with testing requirements.
tests/denops/testdata/with_import_map/import_map.jsonc (1)
1-10: Good addition of JSONC variant with documentation.Having both JSON and JSONC import map files enables comprehensive testing of both formats. The comments clearly explain each mapping's purpose, improving test maintainability.
deno.jsonc (1)
16-17: Appropriate exclusion pattern for test data.The addition follows the established pattern of excluding test data directories from Deno operations, ensuring the import map test files don't interfere with coverage or type checking.
tests/denops/testdata/with_import_map/helper.ts (1)
1-5: Simple and effective helper module for testing.The module provides clear, testable exports that effectively demonstrate import map functionality. The implementation is clean and serves its testing purpose well.
tests/denops/testdata/with_import_map/plugin_with_import_map.ts (1)
1-13: Excellent demonstration of import map usage in Denops plugin.The plugin correctly implements the Denops Entrypoint interface while effectively demonstrating import map functionality. The usage of imported helpers and proper async patterns align well with Denops conventions.
denops/@denops-private/service.ts (6)
1-5: LGTM! Clean refactoring to use external Plugin class.The import changes properly introduce the external
Pluginclass while maintaining all necessary type imports.
41-64: Well-structured delegation to Plugin class.The load method correctly instantiates the Plugin class and handles loading lifecycle, including proper cleanup on failure.
66-85: Proper unload delegation with resource cleanup.The unload methods correctly delegate to the Plugin class while ensuring proper cleanup of internal state.
87-93: Clean reload implementation.The reload method correctly preserves the script path from the unloaded plugin for reloading.
112-157: Correct delegation of dispatch calls to Plugin class.The dispatch methods properly delegate API calls to the Plugin class while maintaining error handling and callback mechanisms.
159-174: Proper service shutdown with plugin cleanup.The close method correctly unloads all plugins during service shutdown.
denops/@denops-private/plugin.ts (7)
25-30: Good constructor pattern with immediate loading.The constructor properly initializes the plugin and starts the loading process immediately, which aligns with the established patterns in this codebase.
36-60: Excellent error handling with helpful user guidance.The load method provides comprehensive error handling with specific detection and guidance for Deno module cache issues, which improves the developer experience.
62-88: Robust unload implementation with proper disposal handling.The unload methods correctly implement idempotent behavior and handle disposal errors gracefully while maintaining event atomicity.
90-101: Excellent error handling with stack trace preservation.The call method provides detailed error context by preferring stack traces when available, which greatly aids in debugging.
110-145: Well-designed helper functions with proper error handling.The helper functions provide clean abstractions for script URL handling, event emission, and error detection. The
emitfunction correctly implements the non-throwing behavior as per the codebase conventions.
147-172: Comprehensive import map detection with proper error handling.The function correctly searches for import maps using standard Deno configuration file patterns and handles file system errors appropriately.
174-183: Clean integration of import map support with hot reloading.The function elegantly handles both import map resolution and hot reload suffixes, providing a seamless plugin loading experience.
denops/@denops-private/plugin_test.ts (8)
1-35: Well-organized test setup with proper test data references.The test file properly imports test utilities and references test data using the established
/denops-testdata/import map pattern.
57-72: Good test hygiene with dangling promise prevention.The instantiation test correctly waits for the plugin to load, preventing potential test pollution from unhandled promises.
74-193: Comprehensive coverage of plugin loading scenarios.The tests thoroughly verify event emission, error handling, and special cases like Deno cache issues, ensuring robust plugin loading behavior.
195-344: Thorough testing of unload behavior and edge cases.The tests comprehensively cover disposal patterns, error handling, and idempotent behavior, ensuring reliable plugin unloading.
346-430: Excellent error handling test coverage.The tests properly verify error wrapping, stack trace preservation, and handling of non-Error throws, ensuring robust API call behavior.
432-461: Proper testing of hot reload mechanism.The test correctly verifies that the base script URL remains consistent while allowing for reload suffixes.
463-495: Critical test for system resilience.This test ensures that plugin loading continues even when event emission fails, which is essential for system robustness.
497-570: Comprehensive validation of import map feature.The tests thoroughly verify import map loading, usage in dispatcher calls, and graceful fallback, ensuring the new feature works correctly.
| #denops: Denops; | ||
| #loadedWaiter: Promise<void>; | ||
| #unloadedWaiter?: Promise<void>; | ||
| #disposable: AsyncDisposable = voidAsyncDisposable; |
There was a problem hiding this comment.
Fix variable usage before declaration.
The static analysis tool detected that voidAsyncDisposable is used before its declaration. While JavaScript hoisting might make this work at runtime, it's better to follow proper declaration order.
Move the voidAsyncDisposable declaration before the Plugin class:
+const voidAsyncDisposable = {
+ [Symbol.asyncDispose]: () => Promise.resolve(),
+} as const satisfies AsyncDisposable;
+
export class Plugin {
#denops: Denops;
#loadedWaiter: Promise<void>;
#unloadedWaiter?: Promise<void>;
#disposable: AsyncDisposable = voidAsyncDisposable;And remove the duplicate declaration at lines 104-106.
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🤖 Prompt for AI Agents
In denops/@denops-private/plugin.ts at line 20, the variable voidAsyncDisposable
is used before it is declared, which can cause issues despite JavaScript
hoisting. Move the declaration of voidAsyncDisposable so that it appears before
the Plugin class definition, and remove the duplicate declaration found at lines
104 to 106 to avoid redundancy.
1 participant
Reverts #447
Summary by CodeRabbit
New Features
Refactor
Chores