fix(npm): make ext-content-mdx an optional peer, not a runtime dependency - #3783
Conversation
…ency `npm install veryfront` broke a previously-clean `tsc --noEmit` in any consumer project without skipLibCheck. The root package listed @veryfront/ext-content-mdx under runtime `dependencies`, which drags @mdx-js/mdx -> @types/mdx@2.0.14. That file references the *global* JSX namespace @types/react@19 no longer declares, and tsc auto-includes everything under node_modules/@types, so four TS2503 errors appeared in a package the developer never imported. The issue's suggested `optionalDependencies` does not fix it: npm installs optional dependencies by default and only tolerates their installation *failure*. Verified with npm 11.12.1 in the published package shape -- optionalDependencies installs 117 packages, keeps node_modules/@types/mdx and still exits 2; an optional peer installs 4 packages, has no @types/mdx and exits 0. So the move lands on peerDependencies + peerDependenciesMeta.optional, reusing the mechanism ROOT_OPTIONAL_RUNTIME_PEERS already uses. Three consequences handled: - Server startup calls `ensureBuiltinContentProcessor` unconditionally, so a missing package there would break `npx veryfront dev` for every project, including ones with no .mdx file. It now tolerates a missing-module failure and leaves the contract unregistered, deferring the report to the compile path, which already throws the typed MISSING_EXTENSION_ERROR naming @veryfront/ext-content-mdx. A real load failure inside an installed extension still propagates. - The `minimal` starter ships app/about/page.mdx, so it now declares the extension via `firstPartyExtensions` like docs-agent does for ext-document-kreuzberg. - `update_package_version` only rewrote first-party extension pins under `dependencies`. An RC publish would have left the optional peer pointing at a version that was never published; it now covers optionalDependencies and peerDependencies too. The compiled binary is unchanged: compile-binary.ts embeds extensions/ext-content-mdx/src/index.ts as a compile-time include, with no reference to npm dependency metadata. Refs #3725
|
Warning Review limit reached
Next review available in: 7 minutes Limit details: You’ve used all 3 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe CLI now treats the MDX extension as optional, declares it in MDX scaffolds, normalizes it as an optional peer during publishing, and documents the related installation and TypeScript behavior. Tests cover loading, scaffolding, templates, and package metadata. ChangesMDX extension integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change makes the MDX extension optional while preserving explicit setup for projects that use it and updates related scaffolding and publishing behavior; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant generateCommand
participant packageJson
participant ensureBuiltinContentProcessor
participant MdxContentProcessor
generateCommand->>packageJson: Check generated .mdx files and extension declarations
generateCommand->>ensureBuiltinContentProcessor: Load optional MDX extension
ensureBuiltinContentProcessor->>MdxContentProcessor: Register processor when loading succeeds
ensureBuiltinContentProcessor-->>generateCommand: Defer missing-extension failure to compilation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c915dcb7d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| minimal: { | ||
| firstPartyExtensions: ["@veryfront/ext-content-mdx"], | ||
| }, |
There was a problem hiding this comment.
When callers use materializeScaffold({ template: "ai-agent", features: ["mdx"] }) or otherwise combine the MDX feature with any non-minimal starter, assembleFeatureFiles adds the feature's .mdx routes after this starter-only check, while packageJsonOptions.firstPartyExtensions still comes solely from the base template. The generated project therefore omits @veryfront/ext-content-mdx, and its newly scaffolded MDX routes fail at runtime. Derive the required extension from the assembled feature files or propagate it from the MDX feature configuration rather than covering only starter-owned files.
Useful? React with 👍 / 👎.
| try { | ||
| module = await load(); | ||
| } catch (error) { | ||
| if (isMissingFirstPartyExtensionModule(error, CONTENT_MDX_SPECIFIERS)) return; |
There was a problem hiding this comment.
In an npm project configured for the pages router, veryfront generate page ... and veryfront generate layout ... still create .mdx files in cli/scaffold/engine.ts, but the generate command neither adds @veryfront/ext-content-mdx to package.json nor tells the user to install it. After this missing-package path returns successfully, veryfront dev reports the generated route as unusable even though the command reported that it was created successfully. Update the MDX-generating flow to install or declare the newly optional peer.
Useful? React with 👍 / 👎.
… template Addresses the first review finding on #3783, though not by the route it described. The finding claimed the `mdx` feature adds `.mdx` routes on top of any template, leaving them undeclared. Checked, and that mechanism does not exist: `templates/features/mdx/feature.json` has no `files` key, so the feature scaffolds nothing. `materializeScaffold({template:"ai-agent", features:["mdx"]})` yields 15 files and zero `.mdx`. The gap underneath it is real, though. The feature sets `mdx.enabled` in the generated config and tips the user to "Create .mdx files in app/ directory". Since the extension became an optional peer, a user following that tip on any template gets a runtime failure with nothing in package.json to explain it. `firstPartyExtensions` now comes from the assembled project rather than the template config alone, covering both routes into MDX: - a scaffolded `.mdx` file, which is how `minimal` needs it, and - the `mdx` feature being selected, which scaffolds no files at all. This also subsumes the `minimal` special case: the starter is covered because it ships `app/about/page.mdx`, not because it is named in a table. Two tests, one per direction — the feature selected on a template with no `.mdx`, and a template with neither, which must stay undeclared so the fix cannot over-reach. The positive test also asserts the feature ships zero files, so it fails loudly if that ever changes rather than passing for a new reason. Separately noted, not fixed here: `templates/features/mdx/files/app/docs/*.mdx` exists on disk but is never scaffolded, because the loader goes by `feature.json`. Either dead fixtures or a feature-loader bug, and outside this PR's scope.
…xtension Second review finding on #3783, and reproducible: under the pages router, `veryfront generate page` writes `pages/<slug>.mdx` and `generate layout` writes `layouts/<name>.mdx` (cli/scaffold/engine.ts). Now that @veryfront/ext-content-mdx is an optional peer, a project that never installed it renders those routes as an error — while the command has just printed "Created <path>" and exited 0. The compile path already throws a typed MISSING_EXTENSION_ERROR naming the package, but by then the developer is debugging a route they were told was fine. The warning moves that information to the moment the file is created. Scoped narrowly: it fires only when an `.mdx` file was actually written and the package is absent from all four dependency groups. A project with no readable package.json — a Deno project, say — gets no warning rather than a false one, since the optional-peer problem is npm-specific. The pages router is opt-in via `generate.preferredRouter` or `router` in the config, so this is a narrow path; it is also the only one where the CLI itself creates MDX the user did not ask for by name.
… change `ci (lint)` gates on the generated API reference being current, and `withMdxExtension` moved documented surface in the scaffold module.
|
@codex review Head is now cfc2807. Since your last pass: both of your P1s are addressed — the MDX extension is now derived from the assembled project rather than the template table, and |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfc2807df3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ].some((group) => group?.["@veryfront/ext-content-mdx"] !== undefined); | ||
| if (declared) return; | ||
| cliLogger.warn( | ||
| "This project does not depend on @veryfront/ext-content-mdx, so the generated .mdx file will not render. Install it with: npm install @veryfront/ext-content-mdx", |
There was a problem hiding this comment.
When generate creates an MDX route in a pnpm, Yarn, or Bun project, this warning instructs the user to run npm install, which can create a conflicting package-lock.json while leaving the project's real lockfile stale. The repository already provides detectProjectInstallTarget(projectDir) and formatInstallCommand() in src/extensions/install-command.ts specifically to produce lockfile-aware extension install instructions, so use that path here instead of hard-coding npm.
AGENTS.md reference: AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
Review finding on the head commit, and correct. The warning hard-coded `npm install`, which in a pnpm, Yarn or Bun project writes a competing package-lock.json and leaves the project's real lockfile stale. `detectProjectInstallTarget(projectDir)` and `formatInstallCommand()` in src/extensions/install-command.ts already exist for exactly this — the former reads the lockfile in the directory, falls back to `deno.json`, and searches ancestors for a workspace lockfile before assuming npm. The warning now routes through them instead of guessing. Also replaced the two remaining literal package names with the existing MDX_EXTENSION_PACKAGE constant so the check and the message cannot drift.
|
@codex review Head is 0bea8bd (last pass was cfc2807). Your install-command finding is addressed: the warning now routes through |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Review: 92/100 — mergingThe finding that matters most: this PR contradicts its own issue#3725 proposes
So the change lands on Three consequences, each found rather than guessed
Review findings, both addressedMDX declaration missed the feature path (P1). The stated mechanism did not reproduce —
Deductions−5, −3, the compiled-binary guarantee is verified by reading, not by a test. VerdictGreen at head ( |
fix(npm): make ext-content-mdx an optional peer, not a runtime dependency
npm install veryfrontbroke a previously-cleantsc --noEmitin anyconsumer project without skipLibCheck. The root package listed
@veryfront/ext-content-mdx under runtime
dependencies, which drags@mdx-js/mdx -> @types/mdx@2.0.14. That file references the global JSX
namespace @types/react@19 no longer declares, and tsc auto-includes
everything under node_modules/@types, so four TS2503 errors appeared in a
package the developer never imported.
The issue's suggested
optionalDependenciesdoes not fix it: npm installsoptional dependencies by default and only tolerates their installation
failure. Verified with npm 11.12.1 in the published package shape --
optionalDependencies installs 117 packages, keeps node_modules/@types/mdx
and still exits 2; an optional peer installs 4 packages, has no @types/mdx
and exits 0. So the move lands on peerDependencies +
peerDependenciesMeta.optional, reusing the mechanism
ROOT_OPTIONAL_RUNTIME_PEERS already uses.
Three consequences handled:
Server startup calls
ensureBuiltinContentProcessorunconditionally, soa missing package there would break
npx veryfront devfor everyproject, including ones with no .mdx file. It now tolerates a
missing-module failure and leaves the contract unregistered, deferring
the report to the compile path, which already throws the typed
MISSING_EXTENSION_ERROR naming @veryfront/ext-content-mdx. A real load
failure inside an installed extension still propagates.
The
minimalstarter ships app/about/page.mdx, so it now declares theextension via
firstPartyExtensionslike docs-agent does forext-document-kreuzberg.
update_package_versiononly rewrote first-party extension pins underdependencies. An RC publish would have left the optional peer pointingat a version that was never published; it now covers optionalDependencies
and peerDependencies too.
The compiled binary is unchanged: compile-binary.ts embeds
extensions/ext-content-mdx/src/index.ts as a compile-time include, with no
reference to npm dependency metadata.
Refs #3725
The headline: the issue's own suggested fix does not work
#3725 proposes
optionalDependenciesas the first step. That was measured and it does not solve the problem. npm installs optional dependencies by default —optionalmeans "tolerate installation failure", not "skip". Measured with npm 11.12.1 against the published package shape:node_modules/@types/mdxtsc --noEmitoptionalDependenciesSo the change lands on
peerDependencies+peerDependenciesMeta.optional, reusing theROOT_OPTIONAL_RUNTIME_PEERSmechanism the repo already has. Worth correcting in the issue as well as here.Three consequences that fall out of it
These are the parts most likely to bite, and each is handled:
ensureBuiltinContentProcessorunconditionally. A missing package would have brokennpx veryfront devfor every project, including ones with no.mdxat all. It now tolerates a missing-module failure and defers to the compile path, which already throws the typedMISSING_EXTENSION_ERRORnaming the package. A real load failure inside an installed extension still propagates — the tolerance is narrow.minimalstarter shipsapp/about/page.mdx, so it declares the extension viafirstPartyExtensions, matching what docs-agent already does for ext-document-kreuzberg.update_package_versiononly rewrote first-party pins underdependencies. An RC publish would have shipped an optional peer pointing at a version that was never published. Now coversoptionalDependenciesandpeerDependenciestoo. This one would have been a silent publish break.Acceptance against the issue
npm install veryfrontwith noskipLibCheck— measured, exit 0npx veryfront devstill renders.mdx, or fails with an actionable install message — the typedMISSING_EXTENSION_ERRORnames the packagecompile-binary.tsembedsextensions/ext-content-mdx/src/index.tsas a compile-time include and never reads npm dependency metadatadocs/getting-started/add-to-existing-project.mdupdated to drop theskipLibCheckrequirementCloses #3725
Summary by CodeRabbit
New Features
Documentation
Bug Fixes