Fix JSONC loading issue #454
Conversation
WalkthroughUpdated deno.jsonc to bump an importer dependency and add @std/jsonc. Modified plugin.ts to load local import maps via a JSONC parser and schema check by supplying a custom loader to loadImportMap; remote script handling remains unchanged. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin
participant FS as File System
participant JSONC as JSONC Parser
participant Validator as ImportMap Validator
Plugin->>FS: readText(import_map path)
FS-->>Plugin: file content
Plugin->>JSONC: parse(content)
JSONC-->>Plugin: parsed object
Plugin->>Validator: ensure(obj, isImportMap)
Validator-->>Plugin: ImportMap or error
Plugin-->>Plugin: loadImportMap({ loader })
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Poem
✨ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #454 +/- ##
==========================================
+ Coverage 96.50% 96.77% +0.27%
==========================================
Files 11 11
Lines 915 962 +47
Branches 142 143 +1
==========================================
+ Hits 883 931 +48
+ Misses 30 28 -2
- Partials 2 3 +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: 0
🧹 Nitpick comments (2)
denops/@denops-private/plugin.ts (2)
171-176: Add tests for JSONC (comments/trailing commas) import map loadingThis is a behavior change and worth pinning via tests to prevent regressions. I can draft a minimal test harness that writes a temporary deno.jsonc with trailing commas and verifies tryLoadImportMap resolves an ImportMap.
Do you want me to provide a test snippet leveraging the existing test setup?
171-176: Wrap loader errors for clearer diagnostics while preserving NotFound
The custom loader correctly handles JSONC and allows the outer loop to skip missing files. To make parse failures more actionable, catch non-NotFound errors and rethrow with file path context:return await loadImportMap(importMapPath, { loader: (path: string) => { - const content = Deno.readTextFileSync(path); - return ensure(parseJsonc(content), isImportMap); + try { + const content = Deno.readTextFileSync(path); + return ensure(parseJsonc(content), isImportMap); + } catch (err) { + if (err instanceof Deno.errors.NotFound) { + throw err; // let the outer loop try the next pattern + } + const msg = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to parse import map (${path}): ${msg}`); + } }, });• Confirm whether
loadImportMapaccepts an async loader (i.e. one that returns aPromise). If it does, consider switching to the non-blockingawait Deno.readTextFile(path)inside anasyncloader function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
denops/@denops-private/deno.jsonc(1 hunks)denops/@denops-private/plugin.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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:
denops/@denops-private/deno.jsoncdenops/@denops-private/plugin.ts
⏰ 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)
- GitHub Check: test (ubuntu-latest, 2.x, v9.1.0448, v0.10.0)
- GitHub Check: test (ubuntu-latest, 1.x, v9.1.0448, v0.10.0)
- GitHub Check: test (macos-latest, 2.x, v9.1.0448, v0.10.0)
- GitHub Check: test (macos-latest, 1.x, v9.1.0448, v0.10.0)
- GitHub Check: test (windows-latest, 2.x, v9.1.0448, v0.10.0)
- GitHub Check: test (ubuntu-latest, 1.45.0, v9.1.0448, v0.10.0)
- GitHub Check: test (windows-latest, 1.45.0, v9.1.0448, v0.10.0)
- GitHub Check: test (macos-latest, 1.45.0, v9.1.0448, v0.10.0)
- GitHub Check: test (windows-latest, 1.x, v9.1.0448, v0.10.0)
🔇 Additional comments (5)
denops/@denops-private/deno.jsonc (2)
17-17: Adding @std/jsonc import mapping is appropriateEnables robust JSONC parsing (comments/trailing commas) used in plugin.ts. Looks good.
9-9: @lambdalisue/import-map-importer ^0.5.0 bump compatibility confirmed
Onlydenops/@denops-private/plugin.tsimports and callsloadImportMapwith the newloadercallback signature introduced in v0.5.0. There are no other references to pre-0.5 behavior in the codebase.denops/@denops-private/plugin.ts (3)
5-7: Importing isImportMap to type-guard parsed import maps is a solid additionThis ensures only valid ImportMap objects flow into the importer.
8-8: Bringing in ensure from unknownutil is appropriateKeeps runtime validation explicit and typed.
13-13: Using @std/jsonc.parse is the right choice for JSONC filesThis will handle comments and trailing commas reliably.
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores