Harden twoslash config against silent regressions#740
Harden twoslash config against silent regressions#740slang25 wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents TypeScript twoslash type-bundle regressions from silently shipping to the live docs site by (1) failing the Astro build when the generated .twoslash-types/aspire.d.ts bundle is missing and (2) ensuring the frontend dist/ (and twoslash types) are produced during dotnet publish of the StaticHost.
Changes:
- Make
ec.config.mjsthrow (instead of warn) when.twoslash-types/aspire.d.tsis missing to turn a prod-only failure into a build-time failure. - Add a publish-time MSBuild target in
frontend.esprojto runpnpm install --frozen-lockfileandpnpm run buildbefore publishing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/frontend/frontend.esproj | Adds a publish hook intended to ensure pnpm install + pnpm build run during dotnet publish. |
| src/frontend/ec.config.mjs | Fails fast if the twoslash types bundle is missing and always injects the types into twoslash’s VFS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per Copilot review on PR microsoft#740: BeforeTargets="Publish" runs after the Publish target's dependencies, so file enumeration and copy already saw a stale dist/ before our pnpm build ran. Switch to two earlier hooks: - Inject BuildFrontendForPublish into PublishDependsOn so it's the first dep of Publish (idiomatic .NET SDK pattern). - BeforeTargets="PrepareForPublish;ComputeFilesToPublish" as a backstop in case the JS SDK invokes those outside the PublishDependsOn flow. Both fire before any publish-time file collection, ensuring dist/ is fresh when the SDK packages the StaticHost.
Two small defensive changes on top of microsoft#741: - Throw instead of warn when src/data/twoslash/aspire.d.ts is missing. Since microsoft#741 source-controls the bundle, a missing file means the tree is corrupted — catching that at build time beats silently shipping samples that can't resolve `./.modules/aspire.js`. - Flip noErrorValidation to false so unannotated TS errors fail the build rather than rendering as squigglies in the shipped HTML. Samples that deliberately illustrate a compiler error can opt in with `// @errors: <codes>`; otherwise an error means the sample (or the generated SDK shape) is wrong and should be fixed, not shown to readers. Verified with a full build: zero unannotated errors across the current docs.
b2bacf2 to
5eecbb6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!existsSync(ASPIRE_TYPES_PATH)) { | ||
| throw new Error( | ||
| `[ec] src/data/twoslash/aspire.d.ts not found at ${ASPIRE_TYPES_PATH}. ` + | ||
| 'Run `pnpm twoslash-types` to regenerate it from src/data/ts-modules.' | ||
| ); | ||
| } | ||
| const aspireTypes = readFileSync(ASPIRE_TYPES_PATH, 'utf8'); |
There was a problem hiding this comment.
This is a check-then-read pattern (TOCTOU). It’s more robust to read the file directly and throw a tailored error if it fails (e.g., wrap readFileSync in a try/catch). This avoids rare edge cases where the file is deleted/moved between existsSync and readFileSync, and keeps the failure mode consistent.
| ); | ||
| } | ||
| const aspireTypes = readFileSync(ASPIRE_TYPES_PATH, 'utf8'); | ||
|
|
There was a problem hiding this comment.
With the new behavior, an empty (or whitespace-only) aspire.d.ts will still pass and will populate .modules/aspire.ts with empty content, which can re-introduce the ‘silent regression’ this PR is trying to prevent (imports resolve, but types effectively disappear). Consider validating aspireTypes.trim().length > 0 and throwing with a clear message if it’s empty.
| if (aspireTypes.trim().length === 0) { | |
| throw new Error( | |
| `[ec] src/data/twoslash/aspire.d.ts is empty at ${ASPIRE_TYPES_PATH}. ` + | |
| 'Run `pnpm twoslash-types` to regenerate it from src/data/ts-modules.' | |
| ); | |
| } |
| // are declared at `.modules/aspire.ts` so docs samples that import | ||
| // `'./.modules/aspire.js'` resolve against the real API surface. | ||
| extraFiles: aspireTypes ? { '.modules/aspire.ts': aspireTypes } : {}, | ||
| extraFiles: { '.modules/aspire.ts': aspireTypes }, |
There was a problem hiding this comment.
With the new behavior, an empty (or whitespace-only) aspire.d.ts will still pass and will populate .modules/aspire.ts with empty content, which can re-introduce the ‘silent regression’ this PR is trying to prevent (imports resolve, but types effectively disappear). Consider validating aspireTypes.trim().length > 0 and throwing with a clear message if it’s empty.
| // shipping samples that can't resolve `./.modules/aspire.js`. | ||
| if (!existsSync(ASPIRE_TYPES_PATH)) { | ||
| throw new Error( | ||
| `[ec] src/data/twoslash/aspire.d.ts not found at ${ASPIRE_TYPES_PATH}. ` + |
There was a problem hiding this comment.
The error message includes an absolute path, which can leak runner/workspace paths into public CI logs. Consider switching to a relative path in the message (e.g., relative to process.cwd()), while still keeping enough context to debug locally.
|
Hey @slang25 is this still needed? |
Two small defensive changes in
ec.config.mjs, layered on top of #741.Changes
src/data/twoslash/aspire.d.tsis missing. Since Fix twoslash in AppHostBuilder; source-control twoslash .d.ts bundle #741 source-controls the bundle, a missing file means the working tree is corrupted — not a normal path. Catching that at build time beats silently shipping samples that can't resolve./.modules/aspire.js.noErrorValidationtofalseso unannotated TS errors fail the build rather than rendering as squigglies in the shipped HTML. Samples that deliberately illustrate a compiler error can opt in with// @errors: <codes>; otherwise an error means the sample (or the generated SDK shape) is wrong and should be fixed, not shown to readers.Originally this PR also carried a
frontend.esprojpublish-time MSBuild target. That fix is no longer needed: #741 source-controls the generated bundle, sodotnet publishno longer depends on a pre-build type-generation step. Dropped it.Test plan
pnpm build:skip-searchpasses withnoErrorValidation: false— no existing sample has an unannotated TS errordist/has zeroCannot find module/ twoslash-error markerssrc/data/twoslash/aspire.d.tsand re-importingec.config.mjsthrows; restoring it loads cleanlypnpm test:unit:twoslash-typespasses🤖 Generated with Claude Code