From 51c470d366236e7ce11863451038d4b56b796457 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Thu, 6 Aug 2026 18:13:07 +0200 Subject: [PATCH] fix(cli): resolve transitive registryDependencies and surface optional resource env vars - Plugins declaring registryDependencies now pull their full dependency graph on 'appkit add' (previously ignored on the plugin branch and never resolved transitively). - declaredEnvVars walks optional resources too, not just required. - Add regression tests for the registry add resolver (dir had none). Signed-off-by: MarioCadenas --- .../src/cli/commands/registry/add.test.ts | 96 +++++++++++++++++++ .../shared/src/cli/commands/registry/add.ts | 60 ++++++++---- 2 files changed, 140 insertions(+), 16 deletions(-) create mode 100644 packages/shared/src/cli/commands/registry/add.test.ts diff --git a/packages/shared/src/cli/commands/registry/add.test.ts b/packages/shared/src/cli/commands/registry/add.test.ts new file mode 100644 index 000000000..756d39040 --- /dev/null +++ b/packages/shared/src/cli/commands/registry/add.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it, vi } from "vitest"; +import { declaredEnvVars, resolveItems } from "./add"; +import type { RegistryItem } from "./client"; + +function item(name: string, extra: Partial = {}): RegistryItem { + return { name, ...extra }; +} + +describe("declaredEnvVars", () => { + it("collects env vars from required resources", () => { + const manifest = { + resources: { + required: [{ fields: { id: { env: "DATABRICKS_WAREHOUSE_ID" } } }], + }, + }; + expect(declaredEnvVars(manifest)).toEqual(["DATABRICKS_WAREHOUSE_ID"]); + }); + + // Bug #2: optional resources were dropped entirely. + it("also collects env vars from optional resources", () => { + const manifest = { + resources: { + required: [{ fields: { id: { env: "REQUIRED_ENV" } } }], + optional: [{ fields: { id: { env: "OPTIONAL_ENV" } } }], + }, + }; + expect(declaredEnvVars(manifest)).toEqual(["REQUIRED_ENV", "OPTIONAL_ENV"]); + }); + + it("skips fields without an env property", () => { + const manifest = { + resources: { + required: [{ fields: { host: { env: "PGHOST" }, note: {} } }], + }, + }; + expect(declaredEnvVars(manifest)).toEqual(["PGHOST"]); + }); + + it("returns empty for a manifest with no resources", () => { + expect(declaredEnvVars({})).toEqual([]); + }); +}); + +describe("resolveItems", () => { + it("returns requested items in order", async () => { + const fetch = vi.fn(async (name: string) => item(name)); + const result = await resolveItems(["a", "b"], null, fetch); + expect(result.map((i) => i.name)).toEqual(["a", "b"]); + }); + + // Bugs #1 + #3: registryDependencies were ignored on plugins and never + // resolved transitively. + it("resolves transitive registryDependencies", async () => { + const graph: Record = { + a: item("a", { registryDependencies: ["b"] }), + b: item("b", { registryDependencies: ["c"] }), + c: item("c"), + }; + const fetch = vi.fn(async (name: string) => graph[name]); + const result = await resolveItems(["a"], null, fetch); + expect(result.map((i) => i.name)).toEqual(["a", "b", "c"]); + }); + + it("de-duplicates shared dependencies and fetches each once", async () => { + const graph: Record = { + a: item("a", { registryDependencies: ["shared"] }), + b: item("b", { registryDependencies: ["shared"] }), + shared: item("shared"), + }; + const fetch = vi.fn(async (name: string) => graph[name]); + const result = await resolveItems(["a", "b"], null, fetch); + expect(result.map((i) => i.name)).toEqual(["a", "b", "shared"]); + expect(fetch).toHaveBeenCalledTimes(3); + }); + + it("does not loop on circular dependencies", async () => { + const graph: Record = { + a: item("a", { registryDependencies: ["b"] }), + b: item("b", { registryDependencies: ["a"] }), + }; + const fetch = vi.fn(async (name: string) => graph[name]); + const result = await resolveItems(["a"], null, fetch); + expect(result.map((i) => i.name)).toEqual(["a", "b"]); + expect(fetch).toHaveBeenCalledTimes(2); + }); + + it("strips the namespace from dependency refs", async () => { + const graph: Record = { + a: item("a", { registryDependencies: ["@databricks-appkit/b"] }), + b: item("b"), + }; + const fetch = vi.fn(async (name: string) => graph[name]); + const result = await resolveItems(["a"], null, fetch); + expect(result.map((i) => i.name)).toEqual(["a", "b"]); + }); +}); diff --git a/packages/shared/src/cli/commands/registry/add.ts b/packages/shared/src/cli/commands/registry/add.ts index 8361891a8..dd7835768 100644 --- a/packages/shared/src/cli/commands/registry/add.ts +++ b/packages/shared/src/cli/commands/registry/add.ts @@ -10,7 +10,7 @@ import { type RegistryItemFile, stripNamespace, } from "./client"; -import { REGISTRY_REPO, resolveToken } from "./constants"; +import { REGISTRY_REPO, type RegistryToken, resolveToken } from "./constants"; import { registerPluginInServer } from "./server-register"; /** Subdirectories that commonly hold the frontend / server in an AppKit app. */ @@ -25,7 +25,7 @@ interface ManifestResource { } interface PluginManifestShape { name?: string; - resources?: { required?: ManifestResource[] }; + resources?: { required?: ManifestResource[]; optional?: ManifestResource[] }; } function isDir(p: string): boolean { @@ -86,9 +86,14 @@ function resolveUiTarget(base: string, file: RegistryItemFile): string { return path.join(base, target); } -function requiredEnvVars(manifest: PluginManifestShape): string[] { +/** Env var names declared by a manifest's resources (required and optional). */ +export function declaredEnvVars(manifest: PluginManifestShape): string[] { const envs: string[] = []; - for (const res of manifest.resources?.required ?? []) { + const resources = [ + ...(manifest.resources?.required ?? []), + ...(manifest.resources?.optional ?? []), + ]; + for (const res of resources) { for (const field of Object.values(res.fields ?? {})) { if (field.env) envs.push(field.env); } @@ -184,6 +189,39 @@ interface PluginSummary { envs: string[]; } +/** + * Fetches the requested items plus their transitive registryDependencies. + * Dependencies are resolved breadth-first and de-duplicated by name, so a + * plugin that depends on another registry item pulls the whole graph in one + * `add`. Explicitly-requested items keep their request order and come first. + */ +export async function resolveItems( + names: string[], + token: RegistryToken | null, + fetchItem: ( + name: string, + token: RegistryToken | null, + ) => Promise = fetchRegistryItem, +): Promise { + const seen = new Set(); + const ordered: RegistryItem[] = []; + const queue = [...names]; + + while (queue.length > 0) { + const name = stripNamespace(queue.shift() as string); + if (seen.has(name)) continue; + seen.add(name); + const item = await fetchItem(name, token); + ordered.push(item); + for (const dep of item.registryDependencies ?? []) { + const depName = stripNamespace(dep); + if (!seen.has(depName)) queue.push(depName); + } + } + + return ordered; +} + async function runAdd( refs: string[], opts: { force?: boolean; cwd?: string; register?: boolean }, @@ -196,11 +234,7 @@ async function runAdd( ); } - const names = refs.map(stripNamespace); - const items: RegistryItem[] = []; - for (const name of names) { - items.push(await fetchRegistryItem(name, token)); - } + const items = await resolveItems(refs, token); const hasUi = items.some((i) => !isPluginItem(i)); const hasPlugin = items.some(isPluginItem); @@ -241,16 +275,10 @@ async function runAdd( pluginSummaries.push({ importPath: `./${pluginRel}`, exportName: pluginExportName(item), - envs: requiredEnvVars(manifest), + envs: declaredEnvVars(manifest), }); } else { for (const file of item.files ?? []) { - // UI (Option A) items have no registry deps; warn on any a future item adds. - for (const rd of item.registryDependencies ?? []) { - console.warn( - ` Note: "${item.name}" declares registryDependency "${rd}" — add it separately if needed.`, - ); - } writeItemFile( resolveUiTarget(frontendRoot, file), file.content,