Skip to content

Resolve and install transitive registry Dependencies for registry items#414

Open
rakibulism wants to merge 7 commits intoheygen-com:mainfrom
rakibulism:main
Open

Resolve and install transitive registry Dependencies for registry items#414
rakibulism wants to merge 7 commits intoheygen-com:mainfrom
rakibulism:main

Conversation

@rakibulism
Copy link
Copy Markdown

Motivation

  • Components/blocks can declare registryDependencies and the CLI needs to fetch and install those transitive dependencies before installing the requested item.
  • The add flow must remap targets and install dependencies in topological order so projects get all required files without manual steps.
  • The resolver should surface clear errors for missing dependencies and dependency cycles.

Description

  • Added resolveItemWithDependencies to the registry resolver to fetch an item and all transitive registryDependencies in topological order and detect cycles, and kept resolveItem as a thin wrapper around it.
  • Updated runAdd to call resolveItemWithDependencies, remap targets for every resolved item, install each dependency item before the requested item, and return an installed list in the result along with accumulated written files.
  • Exported resolveItemWithDependencies from the registry index and updated imports in the add command to use it instead of resolveItem.
  • Enhanced tests and fixtures: expanded add.test.ts to expect dependency installation and updated registry/resolver.test.ts to test dependency ordering, missing transitive dependency errors, and cycle detection; adjusted mocked fetch and fixture items to simulate registryDependencies.

Testing

  • Ran resolver unit tests in packages/cli/src/registry/resolver.test.ts which exercise listRegistryItems, resolveItem, and the new resolveItemWithDependencies cases for ordering, missing dependency, and cycles, and they passed.
  • Ran integration/unit tests in packages/cli/src/commands/add.test.ts which verify remapped targets, dependency installs, and snippet behavior, and they passed.

Codex Task

Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up — it's a real gap in the CLI and the core algorithm you wrote (DFS topological sort with cycle detection, item cache, and path-aware cycle errors) is solid, standard stuff and well-tested. Leaving request-changes because there are two issues worth fixing before merge. None are on the resolver logic itself; that part is good.

Is this solving a real problem?

Yes, confirmed:

  • registryDependencies is already declared on RegistryItem (packages/core/src/registry/types.ts:58) and in the JSON schema (packages/core/schemas/registry-item.json:67).
  • The existing resolveItem had an explicit TODO for this exact work.
  • This matches the shadcn-cli registry pattern, which is the right reference model for hyperframes add.

No catalog items use registryDependencies today, but scaffolding the install path before they ship with deps is the correct order.


Required changes

1. Remove IMPROVEMENT_OPPORTUNITIES.md

This file is out of scope for the PR. It's a generic repo-improvement roadmap (LOC limits, any audits, font payload strategy, lint-suppression cleanup, etc.) that doesn't relate to transitive dependency resolution and will conflict with any future organized effort on those topics. If you want to share these observations, the right place is a GitHub discussion or individual issues — but please delete this file from the PR so we can merge a focused change.

2. Fix the silent dep discard in fetchRemoteTemplate

resolveItem is now a thin wrapper that calls resolveItemWithDependencies and returns only the last item:

export async function resolveItem(name, options) {
  const items = await resolveItemWithDependencies(name, options);
  return items[items.length - 1];  // discards items[0..n-1]
}

But there's a second caller that wasn't updated: packages/cli/src/templates/remote.ts:43 in fetchRemoteTemplate (used by hyperframes new for template installs). After this PR, if any template ever declares registryDependencies, that path will:

  1. Fetch the template
  2. Fetch all transitive deps (wasted network)
  3. Cache them
  4. Discard the dep list and install only the template file

— a silently incomplete install, no warning, no error. That's a footgun.

Two acceptable fixes; either is fine:

(a) Teach fetchRemoteTemplate about deps (recommended — matches add.ts and is semantically correct since templates can have deps):

export async function fetchRemoteTemplate(templateId: string, destDir: string): Promise<void> {
  const items = await resolveItemWithDependencies(templateId);
  for (const item of items) {
    await installItem(item, { destDir });
  }

  if (!existsSync(join(destDir, "index.html"))) {
    throw new Error(
      `Example \"${templateId}\" installed but missing index.html. The registry item may be malformed.`,
    );
  }
}

(b) Make resolveItem refuse items with deps — throw a clear error when items.length > 1, forcing callers to adopt the plural API. Cleaner in the long run but a bigger behavior change; fine if you prefer it.


Recommended changes

3. Dead null-checks in add.ts

After resolveItemWithDependencies returns, the requested item is guaranteed to be the last element (it's always pushed last, or the function throws). These two checks are unreachable:

const item = resolved[resolved.length - 1];
if (!item) {
  throw new AddError(`Item \"${opts.name}\" not found — registry unreachable or empty.`, \"unknown-item\");
}
// ...
const installTarget = installPlan[installPlan.length - 1] ?? item;

Can drop both. const item = resolved[resolved.length - 1]!; is enough (or refactor to const [item] = resolved.slice(-1);).

4. Add a diamond-dependency test

The current tests cover linear chain, missing dep, and cycle. Missing: the diamond case (A→B, A→C, B→D, C→D should install D exactly once). Your itemCache + visited set handle this correctly — the test would just lock in that behavior. Worth ~15 lines in resolver.test.ts:

it(\"installs shared transitive dependencies only once (diamond)\", async () => {
  mockFetch({
    dependencies: {
      alpha: [],
      beta: [\"alpha\"],
      gamma: [\"alpha\"],
      // delta depends on both beta and gamma, which both depend on alpha
      delta: [\"beta\", \"gamma\"],
    },
  });
  // ... fetch delta, expect [alpha, beta, gamma, delta] with alpha once
});

(Will need the mock extended slightly to serve delta.)


Minor / optional

5. Serial vs. parallel dependency fetching

Inside visit, deps are fetched serially via await visit(dep, [...path, itemName]). For deep graphs this serializes the HTTP fetches for every dep layer. A wide graph could Promise.all siblings, but the cycle-detection bookkeeping (visiting set ordering) is a bit more subtle in that form. Registries are small today, so this is a don't-fix-now — but a one-line comment noting it's deliberately serial for simpler cycle tracking would save a future reader the same thought.

6. hyperframes:example rejection runs after full resolution

The check at the top of runAdd fires after resolveItemWithDependencies has walked the whole dep graph for an example the user wasn't allowed to add anyway. Minor wasted fetch; pre-existing before your PR, not something you need to fix here, but worth an early-return later.


Summary

  • Must: delete IMPROVEMENT_OPPORTUNITIES.md; fix the fetchRemoteTemplate silent-discard.
  • Should: drop the dead null-checks in add.ts; add a diamond test.
  • Nice: note the serial-fetch tradeoff in a comment.

The resolver implementation itself (cycle detection, error messages, topo ordering, item cache) is good work and doesn't need changes. Thanks again — happy to re-review as soon as the two required items are addressed.

…ies-in-repo-zl9r5h

Resolve and install transitive `registryDependencies` for registry items
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