refactor: migrate adapter imports to package exports#795
Conversation
Replace all relative imports (../../src/registry.js, ../../browser/cdp.js, etc.) with package exports (@jackwener/opencli/registry, @jackwener/opencli/errors, etc.) across all 484 adapter files. This decouples adapter import resolution from directory structure: - User CLIs in ~/.opencli/clis/ resolve via node_modules symlink - Internal adapters resolve via Node.js self-referencing - No more shim files needed for import resolution Changes: - package.json: add sub-path exports for all public modules - clis/**: replace relative imports with @jackwener/opencli/... - discovery.ts: simplify ensureUserCliCompatShims to symlink-only - registry-api.ts: export CommandArgs type - Remove root-level shim directories (browser/, download/, pipeline/) - Remove shim entries from tsconfig.json include and package.json files
- discovery.ts: use 'junction' symlink type on Windows (no admin required) - package-exports.test.ts: generalize forbidden patterns to catch any depth of ../ traversal (not just ../../ and ../../../)
Test files still used old relative paths for vi.mock() and vi.importActual() calls. Updated 5 test files to use package exports. Also broadened regression test patterns to catch mock/importActual paths.
|
设计很干净。package exports + self-referencing 是正解,也为后续 monorepo 迁移铺好了路。 提一个加固建议:shim 移除后,symlink 成了用户 CLI 解析的唯一通道。当前如果 symlink 位置已经是普通目录而非 symlink, 建议把 try { await fs.promises.rm(symlinkPath, { recursive: true, force: true }); } catch { /* doesn't exist */ }外层 catch 也值得加一行 |
Addresses review feedback from Astro-Han: - rm() handles both symlinks and stale directories (unlink fails on dirs) - Log a warning when symlink creation fails instead of silent catch
|
Thanks for the review! Both suggestions applied:
Pushed in 4877213. |
Update all documentation, contributing guides, and skills to use @jackwener/opencli/registry instead of ../../src/registry.js. Without this, users following the docs would write adapters with broken imports since the old shim files are no longer created.
|
Post-merge follow-up: the symlink approach has a gap I missed during review. The symlink at This is now surfacing as the remaining failures in #792 (1.6.5). Suggested fix: extend Sorry for missing this in the original review. |
Summary
Replaces all relative imports across 484 adapter files with Node.js package exports (
@jackwener/opencli/registry,@jackwener/opencli/errors, etc.).Before: adapters used fragile relative paths (
../../src/registry.js) that broke whenever the internal directory structure changed (caused #788, #791).After: adapters use stable package exports that resolve through Node.js module resolution, completely decoupled from internal layout.
What changed
../../src/registry.js→@jackwener/opencli/registry, etc.ensureUserCliCompatShimsfrom ~80 lines of shim generation to ~30 lines (just symlink + package.json)CommandArgstype (was missing)browser/,download/,pipeline/) and their tsconfig/package.json entriesUser adapter authoring (new pattern)
Works in both
~/.opencli/clis/(via symlink) and the package itself (via self-referencing).Test plan
@jackwener/opencli/...imports