Repository navigation
fix(app-shell): explicit extensions on every relative specifier, so plain Node can load the published entry - #5383
Merged
Conversation
…lain Node can load the entry (#5357) Node's ESM resolver does not extension-search relative specifiers and `tsc` never rewrites them, so an extensionless `./Foo` in the source stayed extensionless in `dist` and `import('@object-ui/app-shell')` under plain Node died with ERR_MODULE_NOT_FOUND on the package's own first re-export. Every consumer this repository exercises goes through a bundler, which is why it was green. 1271 relative specifiers now carry the extension they emit as (1227 `.js`, 44 `/index.js`). Every one was identified by the TypeScript PARSER and resolved against the FILESYSTEM: the suffix follows whichever candidate file exists, never the shape of the string. Three strings that parse as imports live in prose comments and were left alone. The parser was used rather than the gate's own regex because the two disagree: `withoutCommentedCode` strips block comments with a pattern that is not line-comment aware, so a back-ticked `@objectstack/*` inside a `//` line opens a block comment it runs to the next close, swallowing two live `import` lines in DraftChangesPanel.tsx. Parser 1271, gate regex 1270, and the one the gate cannot see is real code. Filed separately; not fixed here. `@object-ui/app-shell` was the last name in SPECIFIER_DEBT, so deleting it empties the map: the specifier leg stops being a ratchet with grandfathered names and becomes a hard requirement. The entry still does not evaluate under plain Node, for a different reason that the specifier error was masking: it statically imports @object-ui/plugin-dashboard, whose module-scope react-grid-layout stylesheet Node cannot load at all. That is LOAD_DEBT's class, not this one, and it is recorded there beside the package it comes from. Two consumers outside the package needed the same convention applied: `check-i18n-call-site-keys.mjs` resolved `./i18n.js` to a path with no file behind it and reported the whole metadata-admin table as an unregistered translator, and one source-text pin in @object-ui/i18n spelled the old specifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
…js` specifier to its source (#5357) `resolveModule()` probed candidate extensions but had no notion that `./Foo.js` is how a package whose build preserves specifiers has to spell `Foo.tsx`. So every converted package became partially unwalkable: the walk followed the package entry, then failed on each `./X.js` re-export inside it and stopped. The relative branch of the walk drops an unresolvable specifier with NO record — `if (resolved) walk(resolved); continue;` — so `closure.unresolvable` could never see this class and stayed empty. The only assertion that could catch it was the `filesWalked` floor, and only once enough of the graph had gone. Measured with the walker's own logic: origin/main, as shipped 890 files walked, 236 relative specifiers dropped this branch, as shipped 401 files walked, 275 dropped (below the 500 floor) either tree, repaired 1242 files walked, 0 dropped So this was landing green since objectui#4538 and continued through the six packages of objectui#5214; app-shell converting is what finally crossed the floor. The floor did its job — two PRs late, because the direct symptom was being swallowed. The lookup is a pure FALLBACK after every existing candidate has failed, so a package that genuinely ships a `.js` beside its TypeScript still resolves to that file, and any specifier that already resolved returns the same answer. Repaired, both trees walk an identical 1242 files and reach the same 29 packages with 0 missing alias entries — the alias table was already complete, so the truncated walk was not hiding a second defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
Collaborator
Author
|
Not accepted — CI is red, and it is a real regression from this PR. PM round 19, session
The failing assertion is the one designed for exactly thisIts own preceding comment: "Same guard for the walker: a resolution regression that walked … would make the closure assertion trivially true." The floor exists to catch a walker that quietly stopped resolving, and Diagnosis, offered to the dev to verify rather than adoptThis appears to be the same defect class this PR already found and fixed once:
The interesting part is why the sweep missed itThe PR enumerated its consumption radius mechanically and well: 625 rewritten specifiers searched, plain and regex-escaped, across all 66 files repo-wide that address But this consumer names no app-shell path. It reaches app-shell transitively, through the alias table, via its own resolver. So the blind spot has a shape: a resolver that walks the graph, versus a file that mentions it. I have asked for that class to be enumerated before the fix lands, because one such consumer existing means the class is real. That is a correction to the method, not a criticism of it — mechanical radius enumeration is still far better than waiting for red, and it is what caught the two Constraints given⛔ Do not lower the 500 floor and do not relax the assertion — weakening a gate to get green is out of bounds. If the true post-fix count is legitimately different, report the measurement and route it rather than adjusting it. Everything else in my review stands: surface verified against the declared set plus three declared breaches, Generated by Claude Code |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
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.
Fixes #5357
Node's ESM resolver does not extension-search relative specifiers and
tscnever rewrites them, so an extensionless./Fooin the source stayed extensionless indist. Everything this repository exercises goes through a bundler, which is why it was green.Scale, honestly framed. This is the largest package in the family and the smallest incremental unlock in it. The load leg does not move (see below); what moves is that a published entry stops carrying the defect, and that the ledger guarding the defect class becomes empty.
What changed
1271 relative specifiers across 275 files now carry the extension they emit as. Plus four consumers of that convention, listed under "Beyond the card's file surface".
How the specifiers were chosen — parser, then filesystem
Two decisions, neither made by pattern:
ts.createSourceFileplus a walk overImportDeclaration/ExportDeclaration/ImportTypeNode/ dynamic-import call nodes. Prose is trivia to a parser, so a specifier living in a comment is not a question.resolvesToModuleuses, and the suffix follows whichever candidate answered —./xbecomes./x.jsonly becausex.tsxexists,./x/index.jsonly becausex/index.tsdoes. 0 specifiers resolved to nothing; 0 already carried an extension.Three strings in this package parse as imports but live in prose comments. They were left alone:
The parser and the gate disagree by exactly one, and the gate is wrong
withoutCommentedCode()blanks block comments with a regex that is not line-comment aware.DraftChangesPanel.tsx:44is a line comment naming a package glob:The
/*in that glob opens a fake block comment that runs to the next close, around line 55 — blanking the liveimport { diffFields } from '../views/metadata-admin/previews/object-fields-io'on line 52.withoutCommentedCode(source)Set difference: exactly one, one direction, and it is real code. The gate produced zero false positives. This PR corrects the specifier itself (app-shell source, in scope) but leaves the gate alone — filed as #5382, sibling of #5367. Had the sweep trusted the gate,
distwould have shipped one extensionless import andcheck:esm-specifierswould have called it clean.The diff, graded mechanically
A sweep this size is unreviewable by eye and stays green either way, so the diff was checked by a script that never consults the one that produced it — it reads
git diff -U0, pairs each removed line with its added line, and requires the pair be byte-identical once every quoted string is blanked:git diff --statagrees: 1271 insertions, 1271 deletions. No line added, moved or dropped.The ledger: emptied, and what that does
@object-ui/app-shellwas the last name inSPECIFIER_DEBT, so deleting it leavesnew Map(). Checked rather than assumed, because the card asked:.has()or iteration, both fine on an empty map. The behavioural change is the point — the specifier leg stops being a ratchet with grandfathered names and becomes a hard requirement.scripts/__tests__/check-node-esm-load.test.ts): 28 tests, all pass. Two become vacuous (the per-entry reason loop, and the disjointness loop readingSPECIFIER_DEBT) — worth knowing, not worth blocking on.new Map()resolves to theMap(any, any)overload inlib.es2015.collection.d.ts, sopnpm type-check:scriptsis green with or without an annotation. A@typeannotation was added anyway, since inference now has nothing to work from.The load leg does not move — the second failure was masked
The card predicted 34 of 39 becoming 35. Measured, it stays 34. The specifier defect is gone; app-shell's first error simply moved past it:
DashboardView.tsxandReportView.tsximport@object-ui/plugin-dashboardstatically, and its module-scope stylesheet is something Node cannot load at all. That isLOAD_DEBT's question, not this defect class — it is the exact first-failure shape #5214 measured when it predicted 5 red and observed 10.So
@object-ui/app-shellmoves fromSPECIFIER_DEBTtoLOAD_DEBT, beside the package the failure comes from, and clears the moment@object-ui/plugin-dashboarddoes. This is the one judgement call in the PR and it is one line to revert. The alternative was to leave the load leg red — and since this PR touchesscripts/check-node-esm-load.mjs, that workflow runs on merge tomain, so leaving it red means landing a knowingly-red alarm.Two consequences worth stating plainly:
app-shell,plugin-dashboard,plugin-map(all.css),create-plugin,runner(not importable by design). Every one is ledgered and explained; none is a specifier defect.LOAD_DEBTmatches by package name, not by error code, so the load leg alone would now also silence a genuine specifier regression in app-shell. The specifier leg — a hard requirement as of this PR — is what guards the class, and the ablation below shows it doing so.The CI failure this PR hit, and what it actually revealed
The first push was red on
Test (shard 3/4):examples/console-starter/test/vite-alias-closure.test.ts,expected 401 to be greater than 500. Reproduced locally and traced before touching anything, and the cause is broader than this PR:That suite walks the example's import graph to prove the Vite alias table covers it. Its
resolveModule()probes candidate extensions but had no notion that./Foo.jsaddressesFoo.tsx. So every package converted to explicit extensions became partially unwalkable — the walk follows the package entry, then fails on each./X.jsre-export inside it and stops. Measured with the walker's own logic:mainbefore this PR (as shipped)So this has been landing green since #4538 and was made worse by #5214's six packages;
maintoday walks 890 of 1242 files and nobody could tell. app-shell converting is what finally crossed the floor.Two things follow, and neither is "lower the floor":
closure.unresolvablenever had a chance. The relative branch drops a miss with no record (if (resolved) walk(resolved); continue;), so that assertion is structurally blind to this whole class and would have passed at 401. Only the floor could see it. That reporting gap is why two earlier PRs landed the damage green, and it is filed as The alias-closure walker drops an unresolvable RELATIVE specifier with no record, soclosure.unresolvableis structurally blind to it and only thefilesWalkedfloor can see the damage #5386 rather than changed here — recording relative misses naively would fail on a legitimate./x.cssimport, so it needs a deliberate shape.Repaired,
mainand this branch walk an identical 1242 files and reach the same 29 packages with 0 missing alias entries — so the truncated walk was not hiding a second defect, and this PR changes the graph's spelling, not its structure.Step 2 (the
nodenextpin) is unavailable here, as the card warnedApplied and measured, then reverted.
tscunder the pin: 1097 errors — 880 TS2305, 215 TS7006 (fallout from the unresolved imports), 2 TS2345 — across 162 files.packages/app-shell/tsconfig.jsonis therefore byte-identical tomain. That is #5365's territory. It namedcomponents;layoutis a second package in the same shape, measured here and added to #5365 as a comment rather than split onto a card of its own. The roottsconfig.jsonwas not touched.For the other side of that question: app-shell builds with a bare emitting
tsc, so this source fix reaches its typings for free —dist/**/*.d.tsnow carries 332 relative specifiers, all with explicit extensions.Beyond the card's file surface
The card scoped this to
packages/app-shell/src/**,packages/app-shell/tsconfig.json, and theSPECIFIER_DEBTentry. Four files outside that were changed, each forced mechanically by the convention this PR applies, each named here for review:scripts/check-i18n-call-site-keys.mjs—resolveImport()did not know that./i18n.jsaddressesi18n.ts, so app-shell's registered local translator resolved to a path with no file behind it andpnpm check:i18n-keyswent red, reporting the wholemetadata-admintable asunregistered-translator. The specifier was right and the resolver was wrong. Fixed by stripping the emitted extension before the candidate walk, source spelling first so a package that really ships a.jsstill resolves to it.scripts/__tests__/check-i18n-call-site-keys.test.ts— a pin for the above, so it cannot regress silently.packages/i18n/src/__tests__/marketplace-preview-namespace-3546.test.tsx— a source-text ratchet readingpackages/app-shell/src/console/marketplace/PluginDisclosure.tsxand matching its import line. Spelling updated to the source; the assertion's subject is unchanged, and it pins the explicit form rather than being made tolerant of both.examples/console-starter/test/vite-alias-closure.test.ts— the CI failure above. Same defect as (1) in a second resolver. Written as a pure fallback after every existing candidate has failed, so a package that genuinely ships a.jsbeside its TypeScript still resolves to that file and any specifier that already resolved returns the same answer; behaviour for extensionless specifiers is unchanged by construction.The blind spot in how (1)–(3) were found, and the sweep that closes it
The first three came from enumerating this change's consumption radius by path: the 625 distinct rewritten specifiers searched, plain and regex-escaped, across all 66 files repo-wide that address
packages/app-shell/srcby path. That method cannot see (4), which names no app-shell path at all and reaches it transitively through an alias table with its own resolver.So the radius was re-enumerated by behaviour instead — every file that both extracts import specifiers programmatically and resolves them by probing candidate extensions. Six members repo-wide:
examples/console-starter/test/vite-alias-closure.test.tsscripts/check-i18n-call-site-keys.mjsscripts/check-node-esm-load.mjsresolvesToModulescripts/__tests__/helpers/config-program.tsscripts/__tests__/side-effects-declaration-consistency.test.tspackages/i18n/src/__tests__/fallback-placeholder-spelling-3512.test.tsTwo near-misses checked and cleared:
apps/console/vite.config.ts(that is Vite's ownresolve.extensions, and Vite handles the convention natively) andpackages/cli/src/utils/workspace-vite.ts(probesindex.{ext}inside a src dir, never a specifier).The property that separates the two affected members is worth naming: loud versus silent on a miss. (1) turned a gate red within minutes. (4) had been degrading through two prior PRs. That is the argument in #5386.
Verification
Everything below is at
b5ded7294, on a full build in a dedicated worktree, run from the repository root with path filters — neverpnpm --filter (pkg) test.Whether a build artifact sits between the edit and the thing under test, per leg:
check-node-esm-loadlegs — they readdist/. So the emitted specifier was read out ofdist/before running the gate every time. Marker: the one specifier the gate cannot see,dist/preview/DraftChangesPanel.js:43.vite-alias-closure— it readspackages/*/src,examples/console-starter/srcandvite.config.ts, all sources. Nothing is built between the edit and the assertion, which is why its ablation is a plain re-run.dist/preview/DraftChangesPanel.js:43dist'../views/…/object-fields-io''../views/…/object-fields-io.js'.js, 2 bare'../views/…/object-fields-io''../views/…/object-fields-io.js'.js, 2 bareThe 2 bare ones after the fix are the JSDoc prose above, which
tsccopies into the emit. Neither is code and Node never resolves them.Gate readings
pnpm check:esm-specifiersledgered @object-ui/app-shell: 1270 extensionlesspnpm check:node-esm-loadERR_MODULE_NOT_FOUND.cssMy own count with the gate's method is 1270; the parser's is 1271. The card's three prior numbers (1259 / 1271 / 1813) came from different methods at different times and are not evidence of anything here.
Ablations, each with its prediction recorded first
Specifier sweep — revert
packages/app-shell/srctoorigin/main, keep the emptied ledger, rebuild, readdistback, run both legs.DraftChangesPanel.tsx:52distLOAD_DEBTmatches by nameThe one gap is the last row, and it is my error rather than a finding: full mode runs the specifier leg first, which had already set the failure flag. The mechanism predicted for the load leg was correct — it printed
ledgeredfor a genuine specifier regression, which is exactly why the note above says the specifier leg is now the one carrying the guard. Both ablation legs were rebuilt and both artifacts read back, so no mutated build output survived into the final measurements.Alias-closure resolver — revert the fallback alone. Predicted: the suite reproduces CI exactly, one test failing at 401 against the floor, the other four passing. Observed:
AssertionError: expected 401 to be greater than 500,1 failed | 4 passed. Restored:5 passed, and the walk reportsfilesWalked: 1242,unresolvable: 0, 29 packages, 0 missing aliases.Suites and gates
b5ded7294: 1722 test files, 21693 passed, 1 skipped, 0 failed. (1/4431 files / 5493;2/4431 / 5639;3/4430 / 5504 — the shard that was red;4/4430 / 5057.)pnpm --filter @object-ui/app-shell type-check— clean (tsc --noEmit && tsc -p tsconfig.test.json, both echoed).pnpm --filter '...@object-ui/app-shell' type-check— downstream direction (console, byo-backend-console, console-starter), 4 projects in scope, 0 errors.app-shell+i18n— 0 errors (pre-existing warnings only).b5ded7294, all PASS:lint:coverage,type-check:coverage,check:spec-symbols,check:action-forward-parity,check:phantom-deps,check:self-import,check:esm-specifiers,check:control-bytes,check:published-dist,check:i18n-keys,check:i18n-drift,check:i18n-dead-keys,check:skills-paths,check:doc-types,check:doc-snippets. Pluschangeset:check.Landing
Changeset is
patch(@object-ui/app-shell). Nomajor, per the 2026-08-19 ruling.content/docs/releases/untouched.Observation on #5214, for the maintainer to act on — not acted on here
#5214 has no outstanding piece left after this. Its PR5369 landed as
Part of, so it stayed open pendingapp-shell, and theSPECIFIER_DEBTstring still namingobjectui#5214is the line this commit deletes. All seven packages it listed are done. Two things to weigh before retiring it: the specifier leg it created is now a hard requirement with an empty ledger, which is the state it was aiming at; but #5365, #5382 and #5386 were all split out of that work and remain open. Recorded as an observation only — no state changed on that card from here.Filed along the way, unassigned: #5382 (the gate's comment mask) and #5386 (the alias walker's silent relative-miss). Added to #5365:
@object-ui/layoutmeasured as a second package in that card's shape.Generated by Claude Code