Skip to content

Commit b145e94

Browse files
dmealingclaude
andcommitted
fix: the two adopter blockers — sqlite apply, and browser bundling (#287)
Both are the same shape: a path the tests structurally could not reach. 1. SQLITE APPLY. `meta migrate --apply` failed on EVERY table rebuild on sqlite — the scaffold's default dialect — with "cannot start a transaction within a transaction". The recreate-and-copy recipe is emitted as a standalone-runnable script carrying its own BEGIN/COMMIT, while the runner already wraps each migration in one Kysely transaction so the change and its ledger row commit together. Worse, it aborted mid-file: a fresh adopter who widened an enum lost a dependent view and got nothing in the ledger. The runner now adapts the file to the transaction it owns — dropping transaction control and rewriting `PRAGMA foreign_keys = OFF` to `PRAGMA defer_foreign_keys = ON`, which is not cosmetic: foreign_keys is a no-op inside a transaction, so the rebuild would otherwise lose FK protection exactly where it needs it. Same division D1 already uses. The emitted file stays a correct standalone script on purpose — it is a committed artifact with other consumers (sqlite3, the Flyway output adapter, deploy scripts). Every existing sqlite rebuild test executes the emitted SQL against the engine directly and never goes through applyPending; proving the SQL is correct cannot prove it is appliable by the tool that ships it. Now gated by three tests driving the real runner: regression, convergence, and atomicity. 2. BROWSER BUNDLING (#287). metadata's root exports MetaDataLoader -> library-sources.ts -> `node:url`, so one VALUE import from it dragged the Node-only loader into every browser bundle. runtime-web imported six LAYOUT_* constants that way, and every generated <Entity>.hooks.ts imports from runtime-web — so no client consuming the generated hooks could build at all. Fixed with a `@metaobjectsdev/metadata/constants` subpath: the fifteen pure constant modules, no node:* in the graph. Values come from there; types may stay on the root, since `import type` is erased. Inlining the strings would have violated the constants discipline, so the fix is a safe import path rather than duplicated literals — and it gives src/constants.ts, the home CLAUDE.md has always documented, a real existence. Bun's test runner resolves the "bun" condition to SOURCE and never bundles, so the failure existed only on the dist path a published consumer resolves. My first reproduction missed for exactly that reason and had to be redone against dist. Gated by a real Bun.build({target:"browser"}) over the BUILT output, a purity check on the constants barrel, and a live demo that bundling the root still fails — so if the root ever becomes browser-safe that is discovered deliberately. Both verified to reproduce the reported errors verbatim with the fix reverted. metadata 2312 pass, migrate-ts 699 pass, client suites green, typecheck clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTGT5ksntpcJDZVJ5VyXHS
1 parent 331e786 commit b145e94

8 files changed

Lines changed: 480 additions & 2 deletions

File tree

CHANGELOG.md

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,75 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
77

88
## [Unreleased]
99

10+
### Fixed — [#287](https://github.com/metaobjectsdev/metaobjects/issues/287): the browser packages could not be bundled at all (npm)
11+
12+
`@metaobjectsdev/metadata`'s package root exports `MetaDataLoader`, which imports
13+
`library/library-sources.ts`, which does `import { fileURLToPath } from "node:url"`. So a
14+
single **value** import from that root dragged the Node-only loader into a browser bundle:
15+
16+
```
17+
error: Browser polyfill for module "node:url" doesn't have a matching export
18+
named "fileURLToPath" … metadata/dist/library/library-sources.js
19+
```
20+
21+
`runtime-web` imported six `LAYOUT_*` constants that way for `buildGrid`, and every
22+
generated `<Entity>.hooks.ts` imports `buildFilterQs` from `runtime-web` — so **no client
23+
consuming the generated hooks could produce a production build**, unconditionally, on
24+
`0.21.3`. Reported by an adopting project.
25+
26+
Fixed with a new **`@metaobjectsdev/metadata/constants`** subpath: a barrel of the fifteen
27+
pure `*-constants.ts` modules with no `node:*` anywhere in its graph. Browser packages
28+
import metamodel **values** from it; **types** may still come from the root, since
29+
`import type` is erased at build time and drags in no runtime dependency. Inlining the
30+
strings instead would have violated the project's constants discipline, so the fix is a
31+
safe import path rather than duplicated literals. The barrel also gives
32+
`packages/metadata/src/constants.ts` — the location CLAUDE.md has always documented as the
33+
home for metamodel constants — a real existence.
34+
35+
**Why no test caught it:** this package's tests run under Bun's *test* runner, which
36+
resolves the `"bun"` export condition to TypeScript **source** and never bundles. The
37+
failure exists only on the `dist` path a published consumer resolves, under a
38+
browser-targeted bundler — so unit tests could pass forever while the package was
39+
unbuildable for its only audience. (The first reproduction attempt here missed for exactly
40+
that reason and had to be redone against `dist`.) Gated now by a real
41+
`Bun.build({ target: "browser" })` over the **built** output, plus a purity check on the
42+
constants barrel and a live demo that bundling the root barrel still fails — so if the root
43+
ever becomes browser-safe, that is discovered deliberately rather than by someone
44+
"simplifying" the import back.
45+
46+
The second half of the report — `codegen-ts-tanstack` emitting no `<Entity>.columns.tsx`
47+
without a `layout.dataGrid` node — is real but separable, and is a documentation gap rather
48+
than a defect; not addressed here.
49+
50+
### Fixed — a SQLite table-rebuild migration could not be applied by the tool that emits it (npm)
51+
52+
`meta migrate --apply` / `apply-pending` failed on **every** table rebuild on sqlite — the
53+
scaffold's default dialect — with `SQLITE_ERROR: cannot start a transaction within a
54+
transaction`. The recreate-and-copy recipe (a column type change, a CHECK or FK change, an
55+
evolved `field.enum @values`) is emitted as a standalone-runnable script carrying its own
56+
`PRAGMA foreign_keys = OFF; BEGIN TRANSACTION; … COMMIT;`, while the apply runner already
57+
wraps a migration's statements in one Kysely transaction so the change and its ledger row
58+
commit together. SQLite rejects the nested `BEGIN` outright.
59+
60+
The failure landed **mid-file**, so leading statements had already run: a fresh adopter who
61+
widened an enum ended up with a dependent view dropped and not recreated, and nothing
62+
recorded in the ledger. Found by a from-scratch adopter walkthrough.
63+
64+
The runner now adapts the file to the transaction it already owns: transaction control is
65+
dropped, and `PRAGMA foreign_keys = OFF` is rewritten to `PRAGMA defer_foreign_keys = ON`
66+
not cosmetic, since `foreign_keys` is a **no-op inside a transaction** and the rebuild would
67+
otherwise lose FK protection exactly where it needs it. This is the same division D1 already
68+
uses. The emitted file is deliberately left correct as a standalone script, because it is a
69+
committed artifact with other consumers (`sqlite3`, the ADR-0015 Flyway output adapter,
70+
hand-rolled deploy scripts).
71+
72+
**Why no test caught it:** every existing sqlite rebuild test executes the emitted SQL
73+
statement-by-statement against the engine directly and never goes through `applyPending`
74+
proving the SQL is correct cannot prove it is appliable by the tool that ships it. Gated now
75+
by three tests driving the real runner: the enum-widening regression, convergence (re-diff
76+
empty after apply), and atomicity (a failing rebuild leaves nothing applied and nothing in
77+
the ledger).
78+
1079
### Fixed — [#286](https://github.com/metaobjectsdev/metaobjects/issues/286): the Hono CRUD helpers 500'd on Postgres (npm)
1180

1281
`runtime-ts`'s Hono `mountCrudRoutes` / `mountReadOnlyRoutes` called Drizzle's `.all()`

client/web/packages/runtime-web/src/grid-from-metadata.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,19 @@
1010
// attrs from the dataGrid layout) so a runtime-built grid matches a generated
1111
// one. Browser-safe: depends only on @metaobjectsdev/metadata.
1212
import type { MetaObject, MetaField, MetaView } from "@metaobjectsdev/metadata";
13+
// #287: metamodel VALUES come from the browser-safe constants subpath, never the package
14+
// root. The root exports MetaDataLoader -> library-sources.ts -> `node:url`, so a single
15+
// constant import from it made every browser bundle fail ("Browser polyfill for module
16+
// node:url doesn't have a matching export named fileURLToPath"). The type import above is
17+
// fine on the root: `import type` is erased at build time and drags in no runtime dep.
1318
import {
1419
LAYOUT_SUBTYPE_DATA_GRID,
1520
LAYOUT_DATA_GRID_ATTR_COLUMNS,
1621
LAYOUT_DATA_GRID_ATTR_PAGE_SIZE,
1722
LAYOUT_DATA_GRID_ATTR_DEFAULT_SORT_FIELD,
1823
LAYOUT_DATA_GRID_ATTR_DEFAULT_SORT_ORDER,
1924
LAYOUT_DATA_GRID_ATTR_FILTERABLE,
20-
} from "@metaobjectsdev/metadata";
25+
} from "@metaobjectsdev/metadata/constants";
2126
import type { GridConfig } from "./fetcher.js";
2227

2328
const DEFAULT_PAGE_SIZE = 25;
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/**
2+
* #287 — the browser packages must actually bundle for a browser.
3+
*
4+
* `@metaobjectsdev/metadata`'s package root exports `MetaDataLoader`, which imports
5+
* `library/library-sources.ts`, which does `import { fileURLToPath } from "node:url"`.
6+
* So a single VALUE import from that root — `runtime-web` imported six `LAYOUT_*`
7+
* constants for `buildGrid` — dragged the Node-only loader into every browser bundle:
8+
*
9+
* error: Browser polyfill for module "node:url" doesn't have a matching export
10+
* named "fileURLToPath"
11+
* … metadata/dist/library/library-sources.js
12+
*
13+
* Because every generated `<Entity>.hooks.ts` imports `buildFilterQs` from
14+
* `runtime-web`, **no client consuming the generated hooks could build at all** —
15+
* unconditionally, on 0.21.3. Reported by an adopting project.
16+
*
17+
* Why nothing caught it: this package's tests run under Bun's *test* runner, which
18+
* resolves the `"bun"` export condition to TypeScript SOURCE and never bundles. The
19+
* failure only exists on the `dist` path a published consumer resolves, under a
20+
* browser-targeted bundler. Unit tests here can pass forever while the package is
21+
* unbuildable for its only audience. So this test does the one thing that reproduces it:
22+
* runs a real browser-target bundle over the BUILT output.
23+
*
24+
* The root barrel already keeps one Node-only module out deliberately
25+
* (`registry-coverage.ts`, with a comment saying why); `library-sources` reaches it via
26+
* the loader instead. The fix is `@metaobjectsdev/metadata/constants` — a barrel of pure
27+
* constant modules with no `node:*` anywhere in its graph — which browser packages import
28+
* VALUES from. Types may still come from the root: `import type` is erased at build time.
29+
*/
30+
31+
import { describe, test, expect } from "bun:test";
32+
import { existsSync } from "node:fs";
33+
import { join } from "node:path";
34+
35+
const PKG_ROOT = join(import.meta.dir, "..");
36+
const DIST_ENTRY = join(PKG_ROOT, "dist", "index.js");
37+
const METADATA_CONSTANTS = join(
38+
PKG_ROOT, "..", "..", "..", "..", "server", "typescript", "packages", "metadata", "dist", "constants.js",
39+
);
40+
41+
/** Bundle `entry` for the browser via Bun's bundler; resolve the failure text, if any. */
42+
async function browserBundle(entry: string): Promise<{ ok: boolean; message: string }> {
43+
const built = await Bun.build({ entrypoints: [entry], target: "browser", throw: false });
44+
return {
45+
ok: built.success,
46+
message: built.logs.map((l) => String(l)).join("\n"),
47+
};
48+
}
49+
50+
describe("#287 — browser bundleability", () => {
51+
test("the BUILT runtime-web entry bundles for a browser target", async () => {
52+
// dist/ is what a published consumer resolves. If it is missing the gate is
53+
// meaningless, so say so rather than skipping quietly.
54+
expect(
55+
existsSync(DIST_ENTRY),
56+
"dist/index.js is missing — run `bun run build` before this gate; " +
57+
"testing src/ would not reproduce #287 (Bun's test runner resolves the `bun` " +
58+
"export condition to TypeScript source and never bundles).",
59+
).toBe(true);
60+
61+
const { ok, message } = await browserBundle(DIST_ENTRY);
62+
// Name the original symptom so a future failure is self-diagnosing.
63+
expect(message).not.toMatch(/node:url|fileURLToPath/);
64+
expect(message).not.toMatch(/node:fs|node:path/);
65+
expect(ok).toBe(true);
66+
});
67+
68+
test("the metadata constants subpath is free of node:* in its whole graph", async () => {
69+
// The fix depends entirely on this barrel staying pure. One transitive node:*
70+
// import added here silently re-breaks every browser build downstream.
71+
expect(existsSync(METADATA_CONSTANTS)).toBe(true);
72+
const { ok, message } = await browserBundle(METADATA_CONSTANTS);
73+
expect(message).not.toMatch(/node:/);
74+
expect(ok).toBe(true);
75+
});
76+
77+
test("importing metamodel VALUES from the metadata ROOT is what breaks — a live demo", async () => {
78+
// Pins the causal claim rather than asserting it in a comment: bundling the root
79+
// barrel for a browser must still fail. If this ever starts passing, the root became
80+
// browser-safe and the constants subpath is no longer load-bearing — worth knowing
81+
// deliberately rather than discovering when someone "simplifies" the import back.
82+
const root = join(
83+
PKG_ROOT, "..", "..", "..", "..", "server", "typescript", "packages", "metadata", "dist", "index.js",
84+
);
85+
if (!existsSync(root)) return; // metadata not built in this run — nothing to assert
86+
const { ok } = await browserBundle(root);
87+
expect(ok).toBe(false);
88+
});
89+
});

server/typescript/packages/metadata/package.json

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@
1515
"bun": "./src/core/index.ts",
1616
"types": "./dist/core/index.d.ts",
1717
"default": "./dist/core/index.js"
18+
},
19+
"./constants": {
20+
"bun": "./src/constants.ts",
21+
"types": "./dist/constants.d.ts",
22+
"default": "./dist/constants.js"
1823
}
1924
},
2025
"files": [
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
/**
2+
* Browser-safe metamodel constants.
3+
*
4+
* Every metamodel string — type names, subtype names, attribute names — lives in a
5+
* per-concern `*-constants.ts` module. This barrel re-exports them and **nothing else**,
6+
* so it can be imported from a browser bundle.
7+
*
8+
* <h3>Why this exists (#287)</h3>
9+
* The package root (`@metaobjectsdev/metadata`) exports `MetaDataLoader`, which imports
10+
* `library/library-sources.ts`, which does `import { fileURLToPath } from "node:url"`.
11+
* So *any* value import from the root barrel — even a single string constant — drags the
12+
* Node-only loader into a browser bundle and the build fails:
13+
*
14+
* error: Browser polyfill for module "node:url" doesn't have a matching export
15+
* named "fileURLToPath" … metadata/dist/library/library-sources.js
16+
*
17+
* That made **every** client consuming the generated TanStack hooks unbuildable, because
18+
* each generated `<Entity>.hooks.ts` imports from `@metaobjectsdev/runtime-web`, which
19+
* imported six `LAYOUT_*` constants from the root. The root barrel already guards one
20+
* Node-only module (`registry-coverage.ts`, kept out deliberately with a comment saying
21+
* why) — `library-sources` reaches it through the loader instead.
22+
*
23+
* <h3>The rule</h3>
24+
* Browser-facing packages (`client/web/**`) import metamodel VALUES from
25+
* `@metaobjectsdev/metadata/constants`, never from the package root. Types may still come
26+
* from the root — `import type` is erased at build time and cannot drag a runtime
27+
* dependency with it.
28+
*
29+
* Inlining the strings instead would violate the project's constants discipline
30+
* ("never inline metamodel strings as literals in code"), so the fix is a safe import
31+
* path rather than duplicated literals.
32+
*
33+
* **Do not add anything to this file that is not a pure constant module.** A single
34+
* transitive `node:*` import here silently re-breaks every browser build; the
35+
* `browser-safe-constants` test asserts that by bundling this entry for the browser.
36+
*/
37+
38+
export * from "./core/attr/attr-constants.js";
39+
export * from "./core/documentation/doc-constants.js";
40+
export * from "./core/field/field-constants.js";
41+
export * from "./core/identity/identity-constants.js";
42+
export * from "./core/index/index-constants.js";
43+
export * from "./core/object/object-constants.js";
44+
export * from "./core/query/query-constants.js";
45+
export * from "./core/relationship/relationship-constants.js";
46+
export * from "./core/validator/validator-constants.js";
47+
export * from "./persistence/db/db-constants.js";
48+
export * from "./persistence/origin/origin-constants.js";
49+
export * from "./persistence/source/source-constants.js";
50+
export * from "./presentation/layout/layout-constants.js";
51+
export * from "./presentation/view/view-constants.js";
52+
export * from "./template/template-constants.js";

server/typescript/packages/migrate-ts/src/apply/apply.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
recordApplied,
1414
} from "./ledger.js";
1515
import { splitSqlStatements } from "../sql/split-statements.js";
16+
import { prepareForRunnerTransaction } from "./runner-transaction-pass.js";
1617

1718
// Re-exported here for back-compat: `splitSqlStatements` historically lived in
1819
// this module. Its canonical home is now ../sql/split-statements.js (shared with
@@ -298,8 +299,14 @@ async function runSqlFileWithLedgerMutation(
298299
sqlText: string,
299300
mutateLedger: (trx: Transaction<Record<string, unknown>>) => Promise<void>,
300301
): Promise<void> {
302+
// Adapt the file to the transaction we are about to open. A SQLite rebuild script
303+
// carries its own BEGIN/COMMIT (correct when piped to `sqlite3`, fatal here — SQLite
304+
// rejects a nested BEGIN, which made table-rebuild migrations un-appliable on the
305+
// scaffold's default dialect) and its own `PRAGMA foreign_keys = OFF`, which is a
306+
// no-op inside a transaction. See runner-transaction-pass.ts for the full rationale.
307+
const { statements } = prepareForRunnerTransaction(sqlText);
301308
await db.transaction().execute(async (trx) => {
302-
for (const stmt of splitSqlStatements(sqlText)) {
309+
for (const stmt of statements) {
303310
await sql.raw(stmt).execute(trx);
304311
}
305312
await mutateLedger(trx);
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
import { splitSqlStatements } from "../sql/split-statements.js";
2+
3+
/**
4+
* Adapt a migration file's statements for execution INSIDE the apply runner's own
5+
* transaction.
6+
*
7+
* <h3>Why this exists</h3>
8+
* A SQLite table rebuild (the recreate-and-copy recipe: a column type change, an
9+
* evolved `field.enum @values`, a CHECK or FK change) is emitted as a **self-contained,
10+
* standalone-runnable script** — `PRAGMA foreign_keys = OFF; BEGIN TRANSACTION; …
11+
* COMMIT; PRAGMA foreign_keys = ON;` — which is the correct recipe when you pipe the
12+
* file into `sqlite3`. But `applyPending` runs the file's statements inside ONE Kysely
13+
* transaction (so the data change and its ledger row commit or roll back together), and
14+
* SQLite rejects a nested `BEGIN` outright:
15+
*
16+
* SQLITE_ERROR: cannot start a transaction within a transaction
17+
*
18+
* The result was that **no table-rebuild migration could be applied at all on sqlite —
19+
* the scaffold's default dialect** — via `--apply`, `apply-pending`, or under Bun. Worse,
20+
* the failure landed mid-file: the leading statements had already run, so a dependent
21+
* view could be dropped and not recreated, leaving the database in a partially-applied
22+
* state with nothing recorded in the ledger. Found by a fresh-adopter test the first time
23+
* an enum gained a member.
24+
*
25+
* <h3>Why the fix is here and not in the emitter</h3>
26+
* Deleting `BEGIN`/`COMMIT` from the emitted file would fix the runner and silently make
27+
* the file non-atomic for anyone executing it directly (`sqlite3 db < up.sql`, a Flyway
28+
* runner via the ADR-0015 output adapter, a hand-rolled deploy script). The file is a
29+
* committed artifact with more than one consumer. So the file stays a correct standalone
30+
* script and the RUNNER adapts it to the transaction it already owns — the same division
31+
* D1 uses, where `applyD1SafetyPass` strips transaction control because D1 wraps the file
32+
* in an implicit transaction.
33+
*
34+
* <h3>What it does</h3>
35+
* 1. **Drops transaction control** (`BEGIN`/`COMMIT`/`ROLLBACK`/`SAVEPOINT`/`RELEASE`).
36+
* The runner's transaction supplies the atomicity the file was asking for.
37+
* 2. **Rewrites `PRAGMA foreign_keys = OFF` to `PRAGMA defer_foreign_keys = ON`.** This is
38+
* not cosmetic: `foreign_keys` is a **no-op inside a transaction**, so the rebuild would
39+
* lose its FK protection precisely where it needs it (dropping a referenced table).
40+
* `defer_foreign_keys` is the in-transaction equivalent — enforcement is deferred to
41+
* commit — and is exactly what the D1 cascade emitter uses for the same reason.
42+
* 3. **Drops the matching `PRAGMA foreign_keys = ON`**, whose only job was to undo (1);
43+
* `defer_foreign_keys` resets itself at commit.
44+
*
45+
* Postgres migrations contain none of these constructs, so this is a no-op for them —
46+
* which is why it keys on statement text rather than needing the dialect threaded in.
47+
*/
48+
export interface RunnerTransactionPassResult {
49+
/** Statements to execute, in order, inside the runner's transaction. */
50+
statements: string[];
51+
/** Human-readable adaptations made, for `--verbose`/diagnostics. Empty when untouched. */
52+
notes: string[];
53+
}
54+
55+
const TRANSACTION_CONTROL = /^\s*(BEGIN|COMMIT|END\s+TRANSACTION|ROLLBACK|SAVEPOINT|RELEASE)\b/i;
56+
const FK_OFF = /^\s*PRAGMA\s+foreign_keys\s*=\s*(OFF|0|false)\s*$/i;
57+
const FK_ON = /^\s*PRAGMA\s+foreign_keys\s*=\s*(ON|1|true)\s*$/i;
58+
59+
export function prepareForRunnerTransaction(sqlText: string): RunnerTransactionPassResult {
60+
const notes: string[] = [];
61+
const statements: string[] = [];
62+
63+
for (const stmt of splitSqlStatements(sqlText)) {
64+
if (TRANSACTION_CONTROL.test(stmt)) {
65+
notes.push(`dropped transaction control (runner owns the transaction): ${firstWords(stmt)}`);
66+
continue;
67+
}
68+
if (FK_OFF.test(stmt)) {
69+
// Preserve the INTENT. A bare foreign_keys pragma does nothing inside a
70+
// transaction, so keeping it verbatim would silently drop FK protection.
71+
statements.push("PRAGMA defer_foreign_keys = ON");
72+
notes.push("rewrote `PRAGMA foreign_keys = OFF` to `PRAGMA defer_foreign_keys = ON` (the in-transaction equivalent)");
73+
continue;
74+
}
75+
if (FK_ON.test(stmt)) {
76+
notes.push("dropped `PRAGMA foreign_keys = ON` (defer_foreign_keys resets at commit)");
77+
continue;
78+
}
79+
statements.push(stmt);
80+
}
81+
82+
return { statements, notes };
83+
}
84+
85+
function firstWords(stmt: string): string {
86+
return stmt.trim().split(/\s+/).slice(0, 3).join(" ");
87+
}

0 commit comments

Comments
 (0)