Repository navigation
fix(build): ship NodeNext-resolvable declarations from every package - #5532
Conversation
tsc copies relative specifiers verbatim, so packages whose sources use bundler-style extensionless imports (core, inspector) published .d.ts files that a module/moduleResolution nodenext consumer cannot resolve. Core already rewrote its emitted JavaScript for this, but not its declarations. Move that rewrite to scripts/rewrite-relative-import-extensions.mjs, make it cover .d.ts too (adding .js or /index.js, and failing the build when a declaration import has no declaration), and run it after every tsc emit: core, engine, sdk, producer, aws-lambda, gcp-cloud-run and inspector. verify:packed-manifests now typechecks the clean packed consumer with skipLibCheck false and type-only imports of every typed subpath, so an unresolvable import inside a published declaration fails the gate and is attributed to its package. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
xuanruli
left a comment
There was a problem hiding this comment.
Approving. One required step before merge: the rebase onto main, which currently conflicts in packages/inspector/build.mjs.
Rebase resolution (required). #5530 merged with an inspector-only rewriter: addDeclarationExtensions and resolveDeclaration at the bottom of packages/inspector/build.mjs. Keep your rewriteRelativeImportExtensions(at("./dist/types")) call, then delete both of those functions and their call. Also drop the imports that only they used: existsSync, readdirSync and writeFileSync from node:fs, and dirname and join from node:path. The @hyperframes/parsers entry #5530 added to .fallowrc.jsonc ignoreDependencies should stay; it's about the dependency, not the rewriter.
I checked that dropping the inspector copy loses nothing. On current main, I built inspector's declarations, then re-emitted them with tsc and ran this PR's scripts/rewrite-relative-import-extensions.mjs over dist/types instead. diff -r of the two trees is empty (18 files rewritten, identical output). That includes the parent-directory imports into the bundled core sources.
What I checked
- The rewriter: file before
indexresolution, which matches TypeScript. Declarations fail loudly when unresolved, while JS is left alone.import()type expressions and side-effect imports are covered.EXPLICIT_EXTENSIONpasses.json/.nodethrough. - The gate:
import type * asplusskipLibCheck: falseis the right fix. The old bareimport "x"withskipLibCheck: trueis exactly how the core and inspector bugs got through. The per-package error count makes a failure easy to route. - Dockerfiles that build core from source now copy the script. The
ALLOWED_DELETIONSentry for the moved core script has a reason. - Locally at
fc8c568:scripts/rewrite-relative-import-extensions.test.mjsandscripts/verify-packed-manifests.test.mjspass (21/21). CI had no failures when I looked.
Non-blocking
- The specifier patterns also match text inside comments. A
.d.tswhose JSDoc happens to containfrom "./something"that doesn't resolve will now fail the build. You already excluded studio for itsimport("./TimelineCanvas")JSDoc link. Since it fails loudly at build time rather than shipping a broken type, I think that's acceptable. It may be worth a line in the script's header so the next person who hits it knows why.
Edit accuracy: accurate 271 (base branch 271), smooth 157 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
fc8c568 to
27bd266
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed at 27bd2663. The #5530 conflict is resolved in the actual head: inspector now has only the shared rewrite call, and its direct-parsers Fallow entry remains.
The shared resolution preserves file-before-directory lookup and explicit runtime extensions (scripts/rewrite-relative-import-extensions.mjs:23–28). The stricter consumer names typed exports and checks dependencies with skipLibCheck: false (scripts/verify-packed-manifests.mjs:339–362), closing the gap that hid the original declaration bug.
Evidence: 21 focused Node tests pass. In an isolated copy of the published core 0.8.150 package, a strict NodeNext consumer importing all 83 typed exports reproduces 94 core errors before the shared rewrite and zero afterward. The second rewrite changes zero files. All 195 published core JavaScript files remain byte-identical after the rewrite. No shared cache/package files were changed.
I read the completed fresh-head Build job 115141763453: all 14 packed manifests and size budgets pass; the clean consumer resolves 166 exports, executes 156 Node exports, and passes the NodeNext declaration check, Vite resolution and CLI startup. Required checks currently reported by GitHub pass; other CI jobs are still running. The cancelled captures attempt was superseded by a passing required captures check. No local Docker/Windows build or full monorepo rebuild.
Prior coverage: Xuanru already covered rewriter edge cases, the inspector equivalence, Docker copy hooks, deletion allowlist and comment matching limitation. No additional blocker from this pass.
Audited: all 17 changed files, seven post-tsc build hooks, source-copy Docker paths, shared resolver and strict gate/fixture. Trusting: full package builds and packed runtime execution from the fresh CI Build log; native rendering behavior.
Verdict: APPROVE
Reasoning: The current head consolidates the inspector fix without losing its dependency setup, repairs published core declarations in a real strict consumer, and adds a gate that detects the original regression.
— Magi
# Conflicts: # package.json
…r changes The package builds now run scripts/rewrite-relative-import-extensions.mjs, so main's edit_accuracy paths filter has to include it (check-edit-accuracy-filter test). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
What
Every package whose JavaScript or declarations come out of tsc now runs one shared post-emit step,
scripts/rewrite-relative-import-extensions.mjs. It gives relative imports in emitted.jsand.d.tsfiles an explicit.jsor/index.js.verify:packed-manifestsnow fails when a published declaration can't be resolved by a strict Node ESM (NodeNext) consumer.Why
tsc copies relative specifiers into its output exactly as written. Core and inspector write bundler-style imports with no extension (
from "./types"), so their published.d.tsfiles carried those imports too. All packages are"type": "module", so a consumer compiling withmodule/moduleResolution: "nodenext"can't resolve them. Magi found this on #5530 (inspector, 16 errors). Core had the same bug, with 136 extensionless imports indist/*.d.ts.Core already rewrote its emitted JavaScript for this (
core_esm_extensions_rewritten), but it skipped declarations. The packed-consumer check already used NodeNext. It missed the bug because it setskipLibCheck: true, which hides errors inside.d.tsfiles, and because it used bareimport "x"side-effect imports, which tsc does not have to resolve.Related work
Refs #5530, which published inspector with its own declaration rewrite in
packages/inspector/build.mjs. This PR removes that local copy, so inspector goes through the shared step like every other tsc-emitting package.How
addDeclarationExtensions/resolveDeclarationfrom build(inspector): publish @hyperframes/inspector to npm #5530 in favor of the shared step.scripts/rewrite-esm-extensions.ts(allowlisted inscripts/check-no-main-deletions.mjs) to the repo root as plain ESM, so packages without tsx can use it. It now covers.d.tsas well as.js. A declaration import resolves againstfoo.d.tsorfoo/index.d.tsand becomes./foo.jsor./foo/index.js. It handlesimport/export ... from, side-effect imports, andimport("./x")type expressions. An unresolvable declaration import fails the build. Unresolvable JavaScript is left alone, as before, because it may sit inside a string, and the existing packed-JS check still rejects a real one.buildscript), producer, aws-lambda, gcp-cloud-run and inspector (build.mjs). The tsup packages (parsers, lint, studio-server, player, shader-transitions, studio) are left out on purpose. Their declarations are bundled by rollup-plugin-dts, which writes its own chunk specifiers with.js. Studio's bundled.d.tsalso hasimport("./TimelineCanvas")inside a JSDoc link, which the strict mode would reject. The gate still covers those packages.Dockerfile.test,packages/gcp-cloud-run/Dockerfile,examples/k8s-jobs/Dockerfile.example) now copy the script. The gcp Dockerfile already copiesscripts/package-subpaths.mjsthe same way.import type * as ... fromfor every published subpath with types. It typechecks withmodule/moduleResolution: NodeNext,skipLibCheck: false,lib: ESNext + DOM, andtypes: ["node"]. It also installs@types/reactand@types/react-domfrom studio's devDependencies, because studio's declarations name React types. A failure lists error counts by owning package. The tsc step takes about 8s instead of 3s.Test plan
NodeNext consumer errors per package (
bun run verify:packed-manifests, using the new gate):tsc stops at the 94 core errors it can reach. A static count finds 136 extensionless specifiers across core's
dist/*.d.tsbefore the change and 0 after.Errors by package: @hyperframes/core: 94. It passes with this change, now covering all 14 published packages (166 exports) including inspector. The inspector "before" count was measured before build(inspector): publish @hyperframes/inspector to npm #5530 merged, by packing it with build(inspector): publish @hyperframes/inspector to npm #5530's manifest and no rewrite..d.tsfiles change. engine, sdk, producer, aws-lambda and gcp-cloud-rundist/are byte-identical to main.scripts/rewrite-relative-import-extensions.test.mjs(new) covers file, directory, parent, side-effect andimport()specifiers, plus the strict-declaration and lenient-JS cases.scripts/verify-packed-manifests.test.mjsadds a real tsc run under the gate's config. It fails on an extensionless declaration import and passes on the.jsone.bun run build,bun run verify:packed-manifests,bun run lint,bun run format:check,bun run test:scripts(737 node tests + 109 vitest), fallow audit, and both comment checks pass.🤖 Generated with Claude Code