👍 Add support for importMap property in deno configuration files #456
Conversation
Previously, the script variable in the Plugin class was always treated as a URL string, but since it was passed as a string type, there was no guarantee that the content was actually a URL. This refactoring ensures type safety by using URL type internally throughout the plugin loading process.
WalkthroughMigrates plugin loading to URL-driven imports, adds deno.json/deno.jsonc and import_map.json[c] import-map resolution (including overrides), updates reload/fragment handling, converts Plugin.script to a getter backed by a private URL, adds tests and testdata for deno.json-based import maps, and updates deno.jsonc excludes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Plugin
participant FS as FileSystem
participant ImportMapImporter
Caller->>Plugin: importPlugin(scriptUrl: URL)
Plugin->>Plugin: resolveScriptUrl(scriptUrl)
Plugin->>Plugin: refreshScriptFragment(scriptUrl)
alt scriptUrl is file:
Plugin->>FS: tryLoadImportMap(dirOf(scriptUrl))
FS-->>Plugin: ImportMap or null
else
Plugin-->>Plugin: skip import-map lookup
end
alt ImportMap available
Plugin->>ImportMapImporter: import(scriptUrl.href, importMap)
ImportMapImporter-->>Plugin: module
else
Plugin->>Plugin: dynamic import(scriptUrl.href)
Plugin-->>Plugin: module
end
Plugin-->>Caller: plugin module loaded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-08-12T20:02:22.038ZApplied to files:
📚 Learning: 2024-09-14T17:09:30.174ZApplied to files:
📚 Learning: 2024-07-08T01:52:22.851ZApplied to files:
⏰ 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 (9)
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #456 +/- ##
==========================================
+ Coverage 96.77% 97.26% +0.48%
==========================================
Files 11 11
Lines 962 986 +24
Branches 143 149 +6
==========================================
+ Hits 931 959 +28
+ Misses 28 24 -4
Partials 3 3 ☔ 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
🧹 Nitpick comments (5)
denops/@denops-private/plugin.ts (2)
158-163: Add context-rich error on JSON parse failure (optional).Parsing errors from JSON/JSONC currently bubble with no file context. Wrapping them with the file URL greatly improves debuggability.
Apply this diff:
async function loadJson(fileUrl: URL): Promise<unknown> { - const content = await Deno.readTextFile(fileUrl); - // Always parse as JSONC to be more permissive - return parseJsonc(content); + const content = await Deno.readTextFile(fileUrl); + // Always parse as JSONC to be more permissive + try { + return parseJsonc(content); + } catch (e) { + throw new Error(`Failed to parse JSON/JSONC: ${fileUrl.href}\n${e instanceof Error ? e.message : String(e)}`); + } }
117-127: Fragment-based cache busting is correct; minor polish optional.The fragment-preserving reload logic works. If you want slightly cleaner fragments, you can avoid nested
#by stripping the leading#fromscriptUrl.hashbefore concatenation.Apply this diff:
- if (loadedScripts.has(scriptUrl.href)) { - // Keep the original fragment and add a timestamp - const fragment = `${scriptUrl.hash}#${performance.now()}`; - return new URL(fragment, scriptUrl); - } + if (loadedScripts.has(scriptUrl.href)) { + // Keep the original fragment (without leading '#') and add a timestamp + const baseFrag = scriptUrl.hash.startsWith("#") + ? scriptUrl.hash.slice(1) + : scriptUrl.hash; + const fragment = `#${baseFrag ? `${baseFrag}#` : ""}${performance.now()}`; + return new URL(fragment, scriptUrl); + }tests/denops/testdata/with_deno_json2/plugin_with_deno_json.ts (1)
1-13: Avoid shell-quoting issues when echoing dynamic strings (test robustness).Embedding unescaped strings in Vim script can break when the string contains quotes. Using JSON.stringify ensures safe quoting in tests.
Apply this diff:
- await denops.cmd(`echo '${message}'`); + await denops.cmd(`echomsg ${JSON.stringify(message)}`);tests/denops/testdata/with_deno_json/plugin_with_deno_json.ts (1)
8-12: Guard against Vimscript quoting issues in echoed stringsUsing single quotes around interpolated values can break if the string contains a single quote. While this is acceptable for a controlled test fixture, a safer pattern is to let Vim parse a JSON string literal.
If you want to harden this (note: tests would need corresponding expectation updates), prefer:
- await denops.cmd(`echo '${message}'`); + await denops.cmd(`echo ${JSON.stringify(message)}`); - await denops.cmd("echo 'Deno json plugin initialized'"); + await denops.cmd(`echo ${JSON.stringify("Deno json plugin initialized")}`);denops/@denops-private/plugin_test.ts (1)
573-652: Avoid dangling plugin instances: unload after each subtestUnloading avoids potential cross-test leakage and keeps lifecycle symmetrical with other tests that cover unload scenarios. Suggested additions are safe since assertions occur before unload.
@@ await t.step("loads plugin with deno.json", async () => { @@ assertSpyCall(_denops_cmd, 0, { args: ["echo 'Deno json plugin initialized'"], }); }); + // Optional: clean up + await plugin.unload(); @@ await t.step("plugin can use mapped imports", async () => { @@ // Should return the greeting from the mapped import assertEquals(result, "Hello from relative import map!"); }); + // Optional: clean up + await plugin.unload(); @@ await t.step("importMap is overridden by imports", async () => { @@ // Should return the greeting from the mapped import assertEquals(result, "Hello from mapped import!"); }); + // Optional: clean up + await plugin.unload();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
deno.jsonc(1 hunks)denops/@denops-private/plugin.ts(7 hunks)denops/@denops-private/plugin_test.ts(2 hunks)tests/denops/testdata/with_deno_json/deno.json(1 hunks)tests/denops/testdata/with_deno_json/other_path/helper.ts(1 hunks)tests/denops/testdata/with_deno_json/other_path/other_name.json(1 hunks)tests/denops/testdata/with_deno_json/plugin_with_deno_json.ts(1 hunks)tests/denops/testdata/with_deno_json2/deno.jsonc(1 hunks)tests/denops/testdata/with_deno_json2/helper.ts(1 hunks)tests/denops/testdata/with_deno_json2/other_path/helper.ts(1 hunks)tests/denops/testdata/with_deno_json2/other_path/other_name.json(1 hunks)tests/denops/testdata/with_deno_json2/plugin_with_deno_json.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 Learning: 2024-09-14T17:09:30.174Z
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.
Applied to files:
tests/denops/testdata/with_deno_json2/deno.jsonctests/denops/testdata/with_deno_json2/helper.tstests/denops/testdata/with_deno_json/deno.jsontests/denops/testdata/with_deno_json/other_path/other_name.jsondeno.jsonctests/denops/testdata/with_deno_json2/other_path/other_name.jsontests/denops/testdata/with_deno_json/plugin_with_deno_json.tstests/denops/testdata/with_deno_json/other_path/helper.tsdenops/@denops-private/plugin_test.tsdenops/@denops-private/plugin.ts
📚 Learning: 2024-07-08T01:52:22.851Z
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.
Applied to files:
denops/@denops-private/plugin.ts
🧬 Code Graph Analysis (6)
tests/denops/testdata/with_deno_json2/plugin_with_deno_json.ts (2)
tests/denops/testdata/with_deno_json/plugin_with_deno_json.ts (1)
main(4-13)tests/denops/testdata/with_deno_json2/helper.ts (2)
getMessage(3-5)greeting(1-1)
tests/denops/testdata/with_deno_json2/helper.ts (2)
tests/denops/testdata/with_deno_json/other_path/helper.ts (2)
greeting(1-1)getMessage(3-5)tests/denops/testdata/with_deno_json2/other_path/helper.ts (2)
greeting(1-1)getMessage(3-5)
tests/denops/testdata/with_deno_json2/other_path/helper.ts (2)
tests/denops/testdata/with_deno_json/other_path/helper.ts (2)
greeting(1-1)getMessage(3-5)tests/denops/testdata/with_deno_json2/helper.ts (2)
greeting(1-1)getMessage(3-5)
tests/denops/testdata/with_deno_json/plugin_with_deno_json.ts (3)
tests/denops/testdata/with_deno_json/other_path/helper.ts (2)
getMessage(3-5)greeting(1-1)tests/denops/testdata/with_deno_json2/helper.ts (2)
getMessage(3-5)greeting(1-1)tests/denops/testdata/with_deno_json2/other_path/helper.ts (2)
getMessage(3-5)greeting(1-1)
tests/denops/testdata/with_deno_json/other_path/helper.ts (1)
tests/denops/testdata/with_deno_json2/helper.ts (2)
greeting(1-1)getMessage(3-5)
denops/@denops-private/plugin_test.ts (2)
tests/denops/testdata/resolve.ts (1)
resolveTestDataURL(9-11)denops/@denops-private/plugin.ts (1)
Plugin(19-109)
🔇 Additional comments (19)
tests/denops/testdata/with_deno_json2/other_path/other_name.json (1)
1-5: Import map entry looks correct and appropriately scopedThe single-specifier mapping and relative target path are valid and align with Deno’s import map resolution (relative to the map file). Good for the test scenario.
deno.jsonc (2)
18-19: Excluding new testdata fixtures is consistent with existing configAdding these directories to exclude is aligned with how other testdata fixtures are handled and should prevent tooling from traversing them inadvertently.
18-19: Top-level “exclude” is applied to all Deno subcommands (check, test, fmt, lint)Deno’s config automatically honors the top-level
"exclude"entries for every subcommand—even when you pass glob patterns on the CLI. In yourdeno.jsonc, the lines:
"tests/denops/testdata/with_deno_json/","tests/denops/testdata/with_deno_json2/",will be excluded from
deno check,deno test,deno fmt, anddeno lintby default. To include them:• Add a negated glob in the appropriate tool-specific section (supported in Deno v1.41.2+). For example:
"test": { "include": ["tests/**/*.ts"], "exclude": [ "tests/denops/testdata/with_deno_json/", "tests/denops/testdata/with_deno_json2/", // “Un-exclude” these paths: "!tests/denops/testdata/with_deno_json/", "!tests/denops/testdata/with_deno_json2/" ] }• Note: A known bug in Deno 2.4.1 may prevent
deno fmtfrom un-excluding certain globs—consider upgrading or adjusting your patterns if you encounter issues.tests/denops/testdata/with_deno_json2/helper.ts (1)
1-5: Test helper content is clear and minimalExports and messaging are straightforward and distinct from the relative-map variant, which helps assert precedence and resolution paths.
tests/denops/testdata/with_deno_json2/other_path/helper.ts (1)
1-5: Relative import-map helper is correctMatches the mapping intent from the external import map file and provides clear, deterministic output for assertions.
tests/denops/testdata/with_deno_json/other_path/helper.ts (1)
1-5: LGTM: mirrors the relative import-map variant for the first test suiteConsistent with the with_deno_json fixtures; exports are minimal and test-friendly.
denops/@denops-private/plugin.ts (4)
171-218: Import map resolution logic is sound and mirrors Deno’s precedence.
- Only attempts for file: URLs.
- Checks deno.json/deno.jsonc first, honors
imports/scopesoverimportMap, then falls back to import_map.json[c].- Stops searching once a config exists but doesn’t yield an import map (matching Deno behavior).
Looks good.
164-170: Good:importMapgating respects Deno precedence.The
hasImportMapPropertyguard correctly ignoresimportMapwhenimportsorscopesexist, aligning with Deno’s “inline imports/scopes take precedence” behavior.
220-229: Importer fallback and URL-based import are clean.
- Uses
ImportMapImporterwhen available; otherwise falls back to dynamic import.- Passes
hrefconsistently.LGTM.
24-30: Publicscriptgetter backed by URL is a safe API refinement.The getter preserves the original shape (
string) while improving internal type safety withURL.tests/denops/testdata/with_deno_json/other_path/other_name.json (1)
1-5: Import map test fixture LGTM.Minimal and correct mapping for the test case.
tests/denops/testdata/with_deno_json2/deno.jsonc (1)
1-8: Config correctly models “imports overrides importMap.”Accurately captures Deno precedence for tests.
tests/denops/testdata/with_deno_json/deno.json (1)
1-3: Config for external import map reference LGTM.Straightforward
importMapreference for the test dataset.tests/denops/testdata/with_deno_json/plugin_with_deno_json.ts (1)
1-13: Test fixture aligns with PR intent and uses Entrypoint + mapped imports correctlyThis plugin is minimal, focuses on mapped imports via deno.json importMap, and matches the expectations asserted in tests. Looks good.
denops/@denops-private/plugin_test.ts (5)
30-35: New test data script constants look correctPaths are resolved via resolveTestDataURL and point to the intended fixtures. No issues.
573-604: “loads plugin with deno.json” — assertions correctly validate load path and eventsThis verifies importMap property support through deno.json and checks event emissions and init echo. Solid coverage.
605-627: “plugin can use mapped imports” — effectively verifies import map resolution via deno.jsonResets spy counts, calls dispatcher, and asserts both echo side-effect and return value. LGTM.
629-651: “importMap is overridden by imports” — confirms Deno precedence semanticsThis ensures imports overrides importMap, matching Deno behavior and PR objectives. Well targeted.
573-652: Import map fixtures configuration verifiedAll of the repository and fixture import-map settings are correct:
- Root
deno.jsoncexcludes both
tests/denops/testdata/with_deno_json/and
tests/denops/testdata/with_deno_json2/.tests/denops/testdata/with_deno_json/deno.jsondeclares
importMap: "./other_path/other_name.json",
and that file exists with the expectedimportsmapping.tests/denops/testdata/with_deno_json2/deno.jsoncdeclares
importMap: "./other_path/other_name.json"
and defines a localimportsblock that correctly overrides the same specifier.No further action required.
|
We'd like to recommend |
|
Wait, I may mis-read. Could you explain what this PR for (rather than what this PR is) |
This adds support for the `importMap` property in deno configuration files. When a deno.json or deno.jsonc file with an `importMap` property, the specified import map file will be loaded and used for module resolution.
I'm added Motivation section to PR top comment. |
|
https://docs.deno.com/runtime/fundamentals/workspaces/#configuring-built-in-deno-tools
Oops, including |
|
So can we close? |
Summary
This PR adds support for the
importMapproperty in Deno configuration files (deno.json / deno.jsonc), allowing plugins to specify external import map files for module resolution.This PR maybe conflicts with #455, so after rebasing if needed.
Motivation
When providing multiple denops plugins in a single repository, each plugin directory
currently requires its own
import_map.jsonfile, leading to dependency duplicationacross related plugins. This makes maintenance difficult as the same dependencies
must be updated in multiple files.
This PR adds support for the importMap property in deno.json/deno.jsonc files,
enabling plugins to reference shared import maps located elsewhere in the repository
structure.
Example use case:
This allows multiple related plugins to share a common import map, reducing
duplication and centralizing dependency management within the repository.
Changes
Support for
importMappropertyimportMapproperty, the specified import map file will be loaded and used for module resolutionimportsorscopesproperties exist in the configuration, they take precedence overimportMap(following Deno's standard behavior)Additionally, Internal refactoring to use URL type
Testing
with_deno_json/: Test case forimportMapproperty referencing an external filewith_deno_json2/: Test case where bothimportMapandimportsexist (importstakes precedence)Summary by CodeRabbit
New Features
Tests
Chores