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
9 changes: 4 additions & 5 deletions .agents/skills/odc-debugging/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,10 @@ prerequisites rather than bugs: `.agents/skills/odc-run-locally/SKILL.md`. Turbo
run outside turbo — a bare `tsc`, `vitest`, storybook — does not.

**Some breakage is known, documented, and deliberately left alone.** Before calling anything a new bug, read the
`AGENTS.md` of the package that owns it — that is where these are recorded (the inverted `catch` in
`packages/instrument-bundler/src/build.ts`, the always-skipped development block in `packages/release-info`,
`testing`'s `test:chrome` naming a Playwright project that does not exist), with the build- and test-infrastructure
ones in the `Known warts` section of `.agents/docs/architecture/testing-strategy.md`. Name any you hit in your reply
and leave it as it is.
`AGENTS.md` of the package that owns it — that is where these are recorded (the always-skipped development block in
`packages/release-info`, `testing`'s `test:chrome` naming a Playwright project that does not exist), with the build-
and test-infrastructure ones in the `Known warts` section of `.agents/docs/architecture/testing-strategy.md`. Name any
you hit in your reply and leave it as it is.

## Quiet and wrong

Expand Down
5 changes: 5 additions & 0 deletions apps/api/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,11 @@ exist.
`libnest.config.ts` copies `@opendatacapture/runtime-v1/dist` and the export worker into `dist/`.
**Any other non-bundled runtime asset must be copied there too, or it will not exist in production.**

`nativeDependencies: ['esbuild']` in `libnest.config.ts` makes the build emit the esbuild native
binary to `dist/` and set `ESBUILD_BINARY_PATH` via the JS banner, so the Dockerfile needs no manual
binary staging. The binary is resolved from the application's dependency graph, not libnest's, so the
JS/binary pair always matches even when the two resolve different esbuild versions.

`#runtime/v1/*` is a Node subpath import declared in `apps/api/package.json` and mirrored in
`apps/api/tsconfig.json` — two files that must agree. See
`.agents/docs/architecture/runtime-and-vendor.md`.
3 changes: 2 additions & 1 deletion apps/api/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ RUN turbo prune @opendatacapture/api
FROM base AS installer
COPY tsconfig.base.json vitest.config.ts ./
COPY --from=pruner /app/out/ .
RUN pnpm install --frozen-lockfile
RUN pnpm install --frozen-lockfile
RUN turbo build --filter=@opendatacapture/api

# RUN SERVER
Expand All @@ -30,5 +30,6 @@ COPY --from=installer /app/apps/api/dist/runtime/ /runtime/

RUN echo '{ "type": "module", "imports": { "#runtime/v1/*": "./dist/runtime/v1/*" } }' > package.json
RUN echo '{ "type": "module" }' > /runtime/package.json

USER node
CMD [ "node", "--enable-source-maps", "./dist/app.js" ]
1 change: 1 addition & 0 deletions apps/api/libnest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ declare module '@douglasneuroinformatics/libnest/user-config' {

const config = defineUserConfig({
build: {
nativeDependencies: ['esbuild'],
onComplete: async () => {
const runtimeV1Dir = path.dirname(
url.fileURLToPath(import.meta.resolve('@opendatacapture/runtime-v1/package.json'))
Expand Down
2 changes: 1 addition & 1 deletion apps/api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"@casl/prisma": "^1.5.1",
"@douglasneuroinformatics/libcrypto": "catalog:",
"@douglasneuroinformatics/libjs": "catalog:",
"@douglasneuroinformatics/libnest": "^8.3.1",
"@douglasneuroinformatics/libnest": "^8.4.0",
"@douglasneuroinformatics/libpasswd": "catalog:",
"@douglasneuroinformatics/libstats": "catalog:",
"@faker-js/faker": "^9.4.0",
Expand Down
6 changes: 5 additions & 1 deletion apps/api/src/instrument-repos/instrument-repos.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,11 @@ export class InstrumentReposService implements OnModuleInit {
}
} catch (err) {
// One bad instrument should not abort importing the rest of the repository.
this.loggingService.error(`Failed to import instrument from ${path.basename(dir)}: ${String(err)}`);
this.loggingService.error({
cause: err,
error: 'Failed to Import Instrument',
instrumentDir: path.basename(dir)
});
}
}

Expand Down
13 changes: 8 additions & 5 deletions packages/instrument-bundler/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,18 @@ degrades every such error, and **esbuild error locations are one line ahead of t
matching `location.lineText` rather
than trusting `location.line`.

**Known defect, verified, unfixed:** the two branches of the `catch` in `build.ts` are inverted. A real
`BuildFailure` parses successfully and is thrown as `'Unknown Error'` with no `kind`, so
`InstrumentBundlerError.isInstance(err, 'ESBUILD_FAILURE')` is never true and `CodeErrorBlock` never
renders. Anything you write that depends on `kind` will not fire until this is corrected.

**Never `import 'esbuild'` directly.** `src/vendor/esbuild.ts` switches between `esbuild` and
`esbuild-wasm` on `typeof window === 'undefined'` — that is what lets the playground bundle in the
browser. Tests also spy on this module (`vi.spyOn(esbuild, 'build')`).

**That switch must initialize its exports in the declaration itself.** `package.json` declares
`sideEffects: ['**/cli.ts']`, so every other file here is advertised as side-effect free and a
bundler may delete any standalone top-level statement. Writing the switch as an `if`/`else` that
assigns to a hoisted `var` puts the initialization in such a statement: `apps/api`'s production
bundle dropped the entire module and every instrument import failed with
`ReferenceError: build is not defined` — a failure that only appears in a bundled build, never under
`pnpm dev`. `src/__tests__/vendor.test.ts` bundles the module with tree shaking to guard this.

**`src/parse.ts` is vendored** from `parse-imports` (Apache-2.0, adapted to TypeScript and to run in a
browser). Treat it as third-party: fix it upstream-style or not at all.

Expand Down
49 changes: 49 additions & 0 deletions packages/instrument-bundler/src/__tests__/build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,55 @@ describe('build', () => {
await expect(build({ inputs })).rejects.toThrowError('expected at most one version of react');
});

// These two branches shipped inverted, which relabelled every real compile failure and meant
// `CodeErrorBlock` never rendered. Both the message and the kind are asserted because each is
// load-bearing: the kind gates the error UI, and the message is what reaches the API log.
it('should report a BuildFailure as a compile failure, so the error UI can render it', async () => {
// esbuild throws a real Error with the diagnostics attached, and the cause must stay that
// error rather than the parsed copy: `InstrumentErrorFallback` gates its `Cause` section on
// `cause instanceof Error`, and libjs `formatError` walks the chain with the same check.
const buildFailure = Object.assign(new Error('Build failed with 1 error'), {
cause: undefined,
errors: [
{
detail: undefined,
id: '',
location: null,
notes: [],
pluginName: '',
text: 'Could not resolve "missing"'
}
],
message: 'Build failed with 1 error',
name: 'BuildFailure',
warnings: []
});
vi.spyOn(esbuild, 'build').mockRejectedValueOnce(buildFailure);
await expect(build(options)).rejects.toSatisfy((err: any) => {
return (
err.name === 'InstrumentBundlerError' &&
err.message === 'Failed to Compile' &&
err.kind === 'ESBUILD_FAILURE' &&
err.cause === buildFailure &&
err.cause instanceof Error &&
err.cause.errors[0].text === 'Could not resolve "missing"'
);
});
});

it('should name anything that is not a BuildFailure in the message, preserving the cause', async () => {
const error = new TypeError('something broke');
vi.spyOn(esbuild, 'build').mockRejectedValueOnce(error);
await expect(build(options)).rejects.toSatisfy((err: any) => {
return (
err.name === 'InstrumentBundlerError' &&
err.message === 'Unexpected error while invoking esbuild: TypeError: something broke' &&
err.kind === undefined &&
err.cause === error
);
});
});

// it('should return javascript that can be executed with no further transformation', () => {
// const result = bundle(options);
// expect((0, eval)(result.js)).toMatchObject({ kind: 'INTERACTIVE' });
Expand Down
28 changes: 28 additions & 0 deletions packages/instrument-bundler/src/__tests__/vendor.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import * as path from 'node:path';

import { describe, expect, it } from 'vitest';

import * as esbuild from '../vendor/esbuild.js';

const VENDOR_DIR = path.resolve(import.meta.dirname, '../vendor');

describe('vendor/esbuild', () => {
it('should keep initializing its exports when tree shaken, so a bundled API does not lose the build binding', async () => {
const result = await esbuild.build({
bundle: true,
external: ['esbuild', 'esbuild-wasm'],
format: 'esm',
keepNames: true,
platform: 'node',
stdin: {
contents: "import * as vendor from './esbuild.js'; export default vendor.build;",
loader: 'ts',
resolveDir: VENDOR_DIR
},
target: ['node22', 'es2022'],
treeShaking: true,
write: false
});
expect(result.outputFiles[0]!.text).toContain('await import("esbuild")');
});
});
13 changes: 10 additions & 3 deletions packages/instrument-bundler/src/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import * as esbuild from './vendor/esbuild.js';

import type { BundlerInput } from './schemas.js';
import type { BuildOutput } from './types.js';
import type { BuildResult } from './vendor/esbuild.js';
import type { BuildFailure, BuildResult } from './vendor/esbuild.js';

const DEFAULT_REACT_PACKAGE = 'react@19.x';

Expand Down Expand Up @@ -37,6 +37,10 @@ function resolveJsxImportSource(inputs: BundlerInput[]): string {
return `/runtime/v1/${packages.values().next().value ?? DEFAULT_REACT_PACKAGE}`;
}

function describeError(err: unknown): string {
return err instanceof Error ? `${err.name}: ${err.message}` : `${typeof err}: ${String(err)}`;
}

function parseBuildResult(result: BuildResult): BuildOutput {
const cssOutput = result.outputFiles?.find((output) => output.path.endsWith('bundle.css'));
const jsOutput = result.outputFiles?.find((output) => output.path.endsWith('bundle.js'));
Expand Down Expand Up @@ -95,9 +99,12 @@ export async function build({
} catch (err) {
const parseResult = await $BuildFailure.safeParseAsync(err);
if (parseResult.success) {
throw new InstrumentBundlerError('Unknown Error', { cause: err });
// the original error, rather than the parsed copy, so that `cause instanceof Error` holds downstream
throw new InstrumentBundlerError('Failed to Compile', { cause: err as BuildFailure, kind: 'ESBUILD_FAILURE' });
}
throw new InstrumentBundlerError('Failed to Compile', { cause: parseResult.error, kind: 'ESBUILD_FAILURE' });
// anything esbuild did not report as a compilation failure is a fault in the bundler itself, not in the
// instrument, so name it here rather than leaving the reader with 'Unknown Error' and a stack
throw new InstrumentBundlerError(`Unexpected error while invoking esbuild: ${describeError(err)}`, { cause: err });
}
return parseBuildResult(result);
}
11 changes: 4 additions & 7 deletions packages/instrument-bundler/src/vendor/esbuild.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
/* eslint-disable no-var */

declare module 'esbuild' {
// eslint-disable-next-line @typescript-eslint/no-unused-vars, @typescript-eslint/consistent-type-definitions
export interface BuildResult<ProvidedOptions extends BuildOptions = BuildOptions> {
Expand All @@ -14,11 +12,10 @@ declare module 'esbuild-wasm' {
}
}

if (typeof window === 'undefined') {
var { build, transform } = await import('esbuild');
} else {
var { build, transform } = await import('esbuild-wasm');
}
// The bindings must be initialized by their own declaration, not by a detached `if`/`else`.
// This package declares `sideEffects: ['**/cli.ts']`, so a bundler is free to drop a
// standalone statement here, which silently leaves `build` undeclared in the output.
const { build, transform } = typeof window === 'undefined' ? await import('esbuild') : await import('esbuild-wasm');

export { build, transform };
export type { BuildFailure, BuildResult, Loader, Location, Message, Plugin } from 'esbuild';
10 changes: 5 additions & 5 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading