Fix denops#cache#update() #461
Conversation
WalkthroughReplaces the batched Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Vim as denops#cache#update()
participant Denops as denops job runner
participant Deno as deno
User->>Vim: call denops#cache#update(reload?)
Vim->>Vim: gather [s:mod] + plugin entry files
loop for each entryfile
Vim->>Vim: build l:args (add --reload if requested)
Vim->>Denops: start job: deno cache <entryfile> (env: NO_COLOR,DENO_NO_PROMPT)
Denops->>Deno: execute deno cache <entryfile>
Deno-->>Denops: exit status
Denops-->>Vim: s:on_exit(failures, entryfile, job, status, event)
Vim->>Vim: record failure if status != 0
end
alt any failures
Vim-->>User: echo warning with failed count and list of entries
else all succeeded
Vim-->>User: echo final success message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
=======================================
Coverage 97.07% 97.07%
=======================================
Files 11 11
Lines 924 924
Branches 146 146
=======================================
Hits 897 897
Misses 24 24
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
autoload/denops/cache.vim(1 hunks)
⏰ 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). (5)
- GitHub Check: test (macos-latest, 2.x, v9.1.1646, v0.11.3)
- GitHub Check: test (ubuntu-latest, 2.x, v9.1.1646, v0.11.3)
- GitHub Check: test (macos-latest, 2.3.0, v9.1.1646, v0.11.3)
- GitHub Check: test (windows-latest, 2.x, v9.1.1646, v0.11.3)
- GitHub Check: test (windows-latest, 2.3.0, v9.1.1646, v0.11.3)
🔇 Additional comments (5)
autoload/denops/cache.vim (5)
9-11: Message on non-reload path — LGTM.Helpful guidance for forcing reload. No change needed.
13-15: Per‑file caching loop — LGTM.Sequentializing isolates failures and matches the PR goal.
16-21: Argument construction — LGTM.Simple and clear; per‑file --reload wiring is correct.
32-32: No-op change.Nothing to flag here.
22-30: Handle per-file failures, respect each plugin’s cwd, and use string env values.Failures/timeouts currently report success and per-plugin deno.json import-maps can be skipped because cwd isn't set; env values should be strings for job portability.
- Initialize: let l:failures = [] before the loop.
- In the loop when starting the job: add cwd: fnamemodify(l:entryfile, ':h') and make env values strings (e.g. '1').
- Capture wait result: let l:done = denops#_internal#wait#for(60 * 1000, { -> s:job is# v:null }, 100).
- If !l:done || s:last_status != 0 then call add(l:failures, l:entryfile).
- At top-level: let s:last_status = 0 and update s:on_exit(job, status, event) to set s:last_status = a:status and let s:job = v:null.
This prevents false “success” and ensures each cache runs from the plugin directory so per‑plugin import‑maps are respected.
|
I don't remember but do we still need this? |
|
The error still exists. I will fix the conflict. |
Use loop
Shougo
force-pushed
the
fix_deno_cache_update
branch
from
b648ed2 to
74266d8
Compare
|
Fixed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
autoload/denops/cache.vim (1)
33-33: Report failures instead of unconditional success.This was already flagged in the previous review. Once you add failure tracking in the loop above, update this line to report errors when files fail to cache.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
autoload/denops/cache.vim (1)
14-32: Consider handling job timeouts explicitly.If a job doesn't complete within 60 seconds,
s:jobwon't be set tov:nullby theon_exitcallback. When the next iteration starts a new job, the timed-out job reference is overwritten. If the timed-out job eventually exits, itson_exitcallback could interfere with a subsequently started job's state.Consider adding explicit cleanup after the wait:
call denops#_internal#wait#for(60 * 1000, { -> s:job is# v:null }, 100) + if s:job isnot# v:null + " Job timed out - add to failures and clean up + call add(l:failures, l:entryfile) + " Optionally: call denops#_internal#job#stop(s:job) if available + let s:job = v:null + endif endfor
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
autoload/denops/cache.vim(2 hunks)
🔇 Additional comments (2)
autoload/denops/cache.vim (2)
8-8: Previous review feedback successfully addressed.The failure tracking initialization and error reporting implementation correctly addresses the previous review comments. The code now collects failures during the loop and provides a clear summary with the count and list of failed entries.
Also applies to: 34-43
54-59: LGTM!The updated callback signature with partial application correctly tracks failures when jobs exit with non-zero status. The implementation cleanly integrates with the failure collection mechanism.
|
The key point of this change is not to ignore errors. By reloading modules one at a time, it avoids producing unnecessary errors. Note: I'm not ignoring errors, just to be clear. |
Then I don’t understand why “reloading modules one at a time” solves the issue. Do you have any perspective on that? I’m just afraid that this “fix” might unintentionally conceal the actual bugs or behaviors of Deno. |
|
I'm not very familiar with the details here either, but it's clear that reloading all the plugins at once causes them not to work properly. Each denops plugin is loading plugins independently via |
|
As long as my understanding, this PR won't fix the issue while https://github.com/lambdalisue/deno-import-map-importer does nothing on |
The error occurs in a plugin that uses |
Use loop to fix import error like this.
Summary by CodeRabbit
Refactor
Bug Fixes