Skip to content

refactor: migrate adapter imports to package exports#795

Merged
jackwener merged 6 commits intomainfrom
refactor/package-exports
Apr 5, 2026
Merged

refactor: migrate adapter imports to package exports#795
jackwener merged 6 commits intomainfrom
refactor/package-exports

Conversation

@jackwener
Copy link
Copy Markdown
Owner

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

  • package.json: added 14 sub-path exports covering all public modules
  • 484 adapter files: ../../src/registry.js@jackwener/opencli/registry, etc.
  • discovery.ts: simplified ensureUserCliCompatShims from ~80 lines of shim generation to ~30 lines (just symlink + package.json)
  • registry-api.ts: exported CommandArgs type (was missing)
  • Removed: root-level shim directories (browser/, download/, pipeline/) and their tsconfig/package.json entries

User adapter authoring (new pattern)

import { cli, Strategy } from '@jackwener/opencli/registry';
import { CliError } from '@jackwener/opencli/errors';
import type { IPage } from '@jackwener/opencli/types';

Works in both ~/.opencli/clis/ (via symlink) and the package itself (via self-referencing).

Test plan

  • Build succeeds (497 entries manifest)
  • All 496 unit tests pass (42 files)
  • Compiled adapters use @jackwener/opencli/... imports

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
Prevents regressions like #788/#791 by:
1. Scanning all adapter files for forbidden relative imports
   (../../src/, ../../browser/, etc.) — fails if any remain
2. Verifying every package.json export maps to an existing source file

18 new test cases.
- 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.
@Astro-Han
Copy link
Copy Markdown
Contributor

设计很干净。package exports + self-referencing 是正解,也为后续 monorepo 迁移铺好了路。

提一个加固建议:shim 移除后,symlink 成了用户 CLI 解析的唯一通道。当前如果 symlink 位置已经是普通目录而非 symlink,unlink() 删不掉目录,symlink() 因 EEXIST 失败,外层 catch 静默吞掉异常,用户侧无任何提示。

建议把 unlink 换成 rm

try { await fs.promises.rm(symlinkPath, { recursive: true, force: true }); } catch { /* doesn't exist */ }

外层 catch 也值得加一行 console.warn,方便排障。

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
@jackwener
Copy link
Copy Markdown
Owner Author

Thanks for the review! Both suggestions applied:

  1. unlink()rm({ recursive: true, force: true }) — handles stale directories that unlink can't remove
  2. Outer catch now logs a warning via log.warn() instead of silently swallowing the error

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.
@jackwener jackwener merged commit 4fe9a73 into main Apr 5, 2026
11 checks passed
@jackwener jackwener deleted the refactor/package-exports branch April 5, 2026 08:02
@Astro-Han
Copy link
Copy Markdown
Contributor

Post-merge follow-up: the symlink approach has a gap I missed during review.

The symlink at ~/.opencli/node_modules/@jackwener/opencli correctly resolves package exports (@jackwener/opencli/registry, etc.), but adapters also import third-party packages as bare specifiers (chalk, turndown). These resolve by walking up from ~/.opencli/clis/<site>/, which never reaches the global node_modules/ where the actual packages live.

This is now surfacing as the remaining failures in #792 (1.6.5).

Suggested fix: extend ensureUserCliCompatShims to also symlink each dependency from package.json into ~/.opencli/node_modules/, derived from the package root's parent directory (the global modules tree).

Sorry for missing this in the original review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants