refactor(schemas): consolidate __dirname-relative schema/data root resolution - #2378
Draft
benminer wants to merge 4 commits into
Draft
Conversation
The tsup ESM banner (node:url/node:path/node:module + __dirname/__filename/ require shims) was applied unconditionally via esbuildOptions to every .mjs output, including pure-data files like enums.generated.mjs that never touch those globals. Strict browser bundlers (Vite, esbuild --platform=browser) can't resolve real Node built-ins, so importing @adcp/sdk/enums or other lean entry points broke bundling for browser consumers. Replace the blanket banner with an esbuild onLoad plugin that only prepends the shim to files whose transpiled body actually references __dirname/__filename/require (11 of 486 files) — detected via a throwaway transformSync so comments merely mentioning "require()" don't false-positive. Runs pre-transform so sourcemaps stay accurate. Add a browser-platform esbuild bundle check to verify-package.mjs so this class of regression fails CI going forward. Fixes #2364
Two follow-ups from review: - require(...features) in SingleAgentClient.ts is a class method, not a call to the global require — the detection regex matched it anyway, giving that file the shim unnecessarily. Every real require() call in the codebase takes a string-literal module specifier, so require the match to have a following quote. - __filename is never read by any source file; it only existed as an intermediate step toward __dirname. Compute __dirname inline instead of binding an unused name.
…solution Replace the hand-tuned `path.join(__dirname, '..', ...)` arithmetic duplicated across 7 schema/data loaders with shared getPackageRoot()/ getSchemaDataRoots() helpers anchored via require.resolve self-reference, independent of each file's own directory depth. No public API changes. Part of ENG-130.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of a follow-up exploration branched off #2365 (ENG-130): the user's instinct on that PR was "maybe a simple banner is the root problem" (comparing against an old project's ampt-style single-bundle esbuild build). Investigation concluded that bundling the whole library per public entry point isn't viable here — 34 public
exportssubpaths, a CLI that deep-requires ~30 non-exporteddist/lib/**paths, and (this PR's target) 7 files that resolve schema/data files via hand-tuned__dirnamearithmetic fanning out across 7+ overlapping public entries.This PR is the first of two pieces from that plan — a standalone cleanup that's worth doing regardless of any bundling question:
src/lib/internal/package-root.ts(getPackageRoot()) andsrc/lib/internal/schema-data-roots.ts(getSchemaDataRoots()).getPackageRoot()resolves the package root viarequire.resolve('@adcp/sdk/package.json')self-reference (falling back to a directory walk), independent of the calling module's own directory depth.path.join(__dirname, '..', ...)arithmetic:validation/schema-loader.ts,conformance/schemaLoader.ts,server/error-arm-tools.ts,v2/projection/{catalog,registry,canonical-properties}.ts,testing/storyboard/compliance.ts. Each file's existing dist-vs-source-tree fallback business logic is unchanged — only the anchor computation changed.schema-loader-per-version.test.js,v2-projection-loader-paths.test.js) that mimicked a published tarball by copying onlydist/, neverpackage.json— unfaithful to a realnpm install(which always shipspackage.json) and would breakgetPackageRoot()'s self-reference.tsup.config.tscomment: the "__dirname-based schema-data lookups keep their original directory depth" justification forbundle: falseno longer applies (tree-shaking and CLI/tooling deep-requires still do —bundle: falseitself is unchanged).No public API changes —
src/lib/internal/is not exported. Branched offissue-2364; that PR (#2365) has since merged, so this branch has been merged up to currentmain.Second piece (CLI build decoupling from
dist/lib, giving the CLI its own esbuild bundle direct from TS source) will follow as a separate PR.Test plan
getPackageRoot()/getSchemaDataRoots()(10 tests, including the directory-walk fallback in isolation)npx tsc --noEmitcleannpm run build:libcleannpm run test:node:fast: 12518/12528 pass. The 3 failures (JWK/OKP crypto errors insigner-grader/request-signing-provider, a timing-sensitive MCP session-cache assertion) are pre-existing — reproduced independently against a stashed pristine baseline, and none of the failing test files reference any of the 7 migrated modules.cli-auth-scheme,cli-webhook-receiver-flag,conformance-cli) in isolation — all 50 tests pass; they looked flaky only under full-suite parallel load.maintests after mergingorigin/main— all green.🤖 Generated with Claude Code