Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .claude/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,17 @@ and rewrites only the RESULT, `<pkg>/dist/<entry>.js` → `<pkg>/src/<entry>.ts`
package, every subpath export, and every package added later is covered with no list to
maintain, and unlike `use-source` it writes nothing into `dist`.

Rewriting the result rather than aliasing has one consequence worth knowing: resolution
still goes through `exports`, which point at `./dist/*`, so **`dist/<entry>.js` must exist
even though the plugin immediately rewrites it to `src`**. When it does not, resolution
fails before the rewrite can happen. The plugin therefore throws its own error naming the
specifier, the owning package and the remedy — and branches on whether `<pkg>/dist` holds
any built entries, because "never built, run `bun run build`" and "a new `exports` subpath
was added without rebuilding, so `dist` is stale" call for different actions and the second
reads as wrong advice to anyone looking at a populated `dist` directory. (`bun run clean`
and `use-dist --no-build` both leave an EMPTY `dist` behind, which is the first case, not
the second.)

This is what makes the numbers mean anything. `packages/test` reaches what it exercises by
package specifier, so with bundles in play v8 attributes those executed lines to
`packages/ai/dist/node.js` and `packages/ai/src/**` reads as barely covered — a package
Expand Down
81 changes: 76 additions & 5 deletions scripts/lib/workspaceSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,71 @@ export function distToSource(id: string): string | undefined {
return undefined;
}

/**
* The workspace package a bare specifier belongs to, or `undefined` when none
* owns it.
*
* Matching is on the package BOUNDARY, not on string prefix: `@workglow/util`
* owns `@workglow/util/schema` but not a hypothetical `@workglow/utilities`.
*/
export function ownerOf(
packages: readonly WorkspacePackage[],
source: string
): WorkspacePackage | undefined {
return packages.find((pkg) => source === pkg.name || source.startsWith(`${pkg.name}/`));
}

/**
* The diagnostic for a workspace specifier that resolved to nothing.
*
* Worth building by hand because the default is actively misleading: this
* plugin rewrites the RESULT of resolution, so resolution itself still goes
* through the package's `exports`, which point at `./dist/*`. With no built
* entry there, resolution fails before the rewrite can happen and the error
* blames the manifest, naming neither this plugin nor anything to do about it.
*
* `distHasBuiltEntries` is passed in rather than probed here so the message
* stays a pure function of its inputs. The two branches need opposite
* responses, which is why they are not one sentence: an empty or absent `dist`
* means the package has simply never been built, whereas a POPULATED `dist`
* that still does not carry this entry is the "added an `exports` subpath and
* did not rebuild" case — where "run build" on its own reads as wrong advice to
* someone looking at a directory full of bundles.
*/
export function unresolvedWorkspaceMessage(
source: string,
owner: WorkspacePackage,
distHasBuiltEntries: boolean,
importer: string | undefined
): string {
const from = importer === undefined ? "" : ` (imported from ${importer})`;
const remedy = distHasBuiltEntries
? `${owner.dir}/dist carries built entries but none for this specifier. A new "exports" ` +
`subpath was most likely added without rebuilding, so dist is stale rather than absent: ` +
`re-run \`bun run build\` (or \`bun run use-source\`).`
: `${owner.dir}/dist is missing or empty — ${owner.name} has never been built in this ` +
`checkout. Run \`bun run build\`, or \`bun run use-source\` to write source stubs into dist.`;
return (
`[workglow:workspace-source] cannot resolve "${source}"${from}. ` +
`It is owned by the workspace package ${owner.name}. Resolution goes through that ` +
`package's "exports", which point at ./dist/*, so the built entry has to exist even ` +
`though this plugin then rewrites it to src. ${remedy}`
);
}

/**
* Whether a package's `dist` holds anything at all. `bun run clean` and
* `use-dist --no-build` both leave the directory in place but empty, which is
* "never built" rather than "stale".
*/
function hasBuiltEntries(packageDir: string): boolean {
try {
return readdirSync(join(packageDir, "dist")).length > 0;
} catch {
return false;
}
}

/**
* Redirect workspace package imports from `dist` to `src`.
*
Expand All @@ -105,9 +170,7 @@ export function distToSource(id: string): string | undefined {
* resolution in an alias table is what a per-package fix would have to do.
*/
export function workspaceSourcePlugin(root: string): Plugin {
const names = listWorkspacePackages(root).map((p) => p.name);
const ownsSpecifier = (source: string): boolean =>
names.some((name) => source === name || source.startsWith(`${name}/`));
const packages = listWorkspacePackages(root);

return {
name: "workglow:workspace-source",
Expand All @@ -116,9 +179,17 @@ export function workspaceSourcePlugin(root: string): Plugin {
// Bare workspace specifiers only: a relative import already points at
// source, and resolving every third-party specifier twice would tax the
// whole run for nothing.
if (!ownsSpecifier(source)) return null;
const owner = ownerOf(packages, source);
if (owner === undefined) return null;
const resolved = await this.resolve(source, importer, { ...options, skipSelf: true });
if (!resolved || resolved.external) return resolved;
if (!resolved) {
// Throwing, not warning: an unresolvable @workglow/* specifier already
// fails the run a moment later. This only replaces the message.
throw new Error(
unresolvedWorkspaceMessage(source, owner, hasBuiltEntries(owner.dir), importer)
);
}
if (resolved.external) return resolved;
const sourceFile = distToSource(resolved.id);
return sourceFile === undefined ? resolved : { ...resolved, id: sourceFile };
},
Expand Down
67 changes: 66 additions & 1 deletion scripts/workspaceSource.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@ import { join } from "node:path";
import { describe, expect, it } from "vitest";
import { stubSpecsFor, type PackageManifest } from "./lib/sourceStubs";
import { ROOT } from "./lib/testDiscovery";
import { distToSource, listWorkspacePackages, WORKSPACE_GROUPS } from "./lib/workspaceSource";
import {
distToSource,
listWorkspacePackages,
ownerOf,
unresolvedWorkspaceMessage,
WORKSPACE_GROUPS,
} from "./lib/workspaceSource";

const packages = listWorkspacePackages(ROOT);

Expand Down Expand Up @@ -84,4 +90,63 @@ describe("workspace source resolution", () => {
);
expect(missing).toEqual([]);
});

/**
* `resolveId` itself needs Vite's plugin context to drive, so the owner
* lookup and the message are separated out and tested directly. The lookup is
* what makes an actionable message possible at all: the old plugin kept only
* package NAMES, so at the point resolution failed it could not say which
* package's `dist` to look at.
*/
describe("owner lookup", () => {
it("attributes a subpath specifier to its package", () => {
expect(ownerOf(packages, "@workglow/util/schema")?.name).toBe("@workglow/util");
expect(ownerOf(packages, "@workglow/util")?.name).toBe("@workglow/util");
});

it("matches on the package boundary, not a string prefix", () => {
// `@workglow/util` is a string prefix of this, but the specifier belongs
// to no package — attributing it would point the diagnostic at an
// unrelated directory.
expect(ownerOf(packages, "@workglow/utilities")).toBeUndefined();
expect(ownerOf(packages, "vitest")).toBeUndefined();
});
});

describe("unresolved specifier diagnostic", () => {
const owner = { name: "@workglow/ai", dir: "/repo/packages/ai" };

it("names the specifier, the owning package and the importer", () => {
const message = unresolvedWorkspaceMessage(
"@workglow/ai/worker",
owner,
false,
"/repo/providers/hft/src/x.ts"
);
expect(message).toContain("@workglow/ai/worker");
expect(message).toContain("@workglow/ai");
expect(message).toContain("/repo/providers/hft/src/x.ts");
expect(message).toContain("workglow:workspace-source");
});

// The two cases need opposite responses, so they must not read the same.
it("distinguishes a never-built package from a stale dist", () => {
const neverBuilt = unresolvedWorkspaceMessage("@workglow/ai/worker", owner, false, undefined);
const staleDist = unresolvedWorkspaceMessage("@workglow/ai/worker", owner, true, undefined);

expect(neverBuilt).toContain("missing or empty");
expect(neverBuilt).toContain("never been built");
// "run build" alone would read as wrong advice to someone looking at a
// populated dist directory, so the stale case has to say why.
expect(staleDist).toContain("carries built entries but none for this specifier");
expect(staleDist).toContain("stale rather than absent");
expect(staleDist).not.toContain("never been built");
});

it("omits the importer clause when there is no importer", () => {
expect(unresolvedWorkspaceMessage("@workglow/ai", owner, true, undefined)).not.toContain(
"imported from"
);
});
});
});
Loading