Skip to content

fix(config): make vite-tsconfig-paths an optional peer#2258

Merged
james-elicx merged 1 commit into
cloudflare:mainfrom
hyoban:codex/optional-vite-tsconfig-paths-peer
Jun 22, 2026
Merged

fix(config): make vite-tsconfig-paths an optional peer#2258
james-elicx merged 1 commit into
cloudflare:mainfrom
hyoban:codex/optional-vite-tsconfig-paths-peer

Conversation

@hyoban

@hyoban hyoban commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • move vite-tsconfig-paths out of vinext runtime dependencies and declare it as an optional peer
  • lazy-load the plugin only for Vite 7, while Vite 8 continues to use native resolve.tsconfigPaths
  • keep vite-tsconfig-paths as a dev dependency for repository tests and update the Vite 7/8 coverage

Why

Vite 8 no longer needs vite-tsconfig-paths, and keeping it as a runtime dependency installs its unmaintained tsconfck transitive dependency for users who do not need the Vite 7 fallback.

Impact

Vite 8 consumers no longer get vite-tsconfig-paths or tsconfck from vinext's runtime dependency tree. Vite 7 consumers still get the existing behavior when they install the optional peer; otherwise vinext reports an actionable error.

Validation

  • pnpm install --lockfile-only --ignore-scripts
  • vp test run tests/tsconfig-paths-vite8.test.ts
  • vp check packages/vinext/src/index.ts packages/vinext/src/deploy.ts tests/tsconfig-paths-vite8.test.ts
  • vp run vinext#build

@hyoban hyoban marked this pull request as ready for review June 22, 2026 08:35
@pkg-pr-new

pkg-pr-new Bot commented Jun 22, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2258
npm i https://pkg.pr.new/vinext@2258

commit: 5da10b2

@github-actions

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 5da10b2 against base cf9d601 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 132.8 KB 132.8 KB ⚫ -0.0%
Client entry size (gzip) vinext 118.7 KB 118.7 KB ⚫ -0.0%
Dev server cold start vinext 2.45 s 2.44 s ⚫ -0.5%
Production build time vinext 2.99 s 2.98 s ⚫ -0.3%
RSC entry closure size (gzip) vinext 92.2 KB 92.2 KB ⚫ +0.0%
Server bundle size (gzip) vinext 166.9 KB 166.9 KB ⚫ +0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5da10b2d41

ℹ️ 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".

// that use @/*, #/*, or baseUrl imports work out of the box.
// Vite 8+ supports this natively via resolve.tsconfigPaths.
...(viteMajorVersion >= 8 ? [] : [tsconfigPaths()]),
...(viteMajorVersion >= 8 ? [] : [loadVite7TsconfigPathsPlugin(earlyBaseDir)]),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid requiring optional alias plugin for all Vite 7 apps

When a Vite 7 project has not installed the new optional peer, this line still creates the loader promise and rejects before vinext:config can inspect whether the app actually has any tsconfig/jsconfig path aliases. That makes plain Vite 7 apps fail to start/build even though vite ^7 remains an accepted peer and getMissingDeps in packages/vinext/src/deploy.ts:845-875 does not install vite-tsconfig-paths for generated deploys. Consider only requiring the peer when aliases are present, or keep a bundled/no-op fallback for Vite 7 projects that do not need alias support.

Useful? React with 👍 / 👎.

@james-elicx

Copy link
Copy Markdown
Member

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: make vite-tsconfig-paths an optional peer

The Vite 8 direction is sound — Vite 8 uses native resolve.tsconfigPaths, so dropping the runtime dep (and its unmaintained tsconfck transitive) is a real win and is well covered.

The concern is the Vite 7 path, which I think regresses harder than the PR description implies.

1. Vite 7 apps now hard-fail at startup if the optional peer is missing — even when they need no aliases

Previously vite-tsconfig-paths was a runtime dependency, so tsconfigPaths() was always present for Vite 7. Now loadVite7TsconfigPathsPlugin(earlyBaseDir) is created eagerly and unconditionally for every Vite 7 app, and rejects when the peer isn't installed. Vite's asyncFlatten awaits that promise during config resolution, so the rejection throws and breaks dev/build for all Vite 7 consumers who didn't install the peer — including apps with zero path aliases. This matches the Codex P2.

What makes this worse: vinext already materializes simple tsconfig/jsconfig path aliases into resolve.alias via resolveTsconfigAliases(root) (index.ts:1252, applied at index.ts:2054) for all Vite versions. So for apps with no aliases — or only simple @/*-style aliases — the hard failure is unnecessary; the native fallback would have covered them. The plugin should only be required when there are aliases that resolveTsconfigAliases can't handle, or it should degrade to a no-op/warning instead of throwing.

Also note getMissingDeps in deploy.ts:845 does not add vite-tsconfig-paths, so vinext-generated Vite 7 deploys won't install it and will hit this failure. If the Vite 7 plugin stays a hard requirement, deploy detection should add it for Vite 7 projects — but a graceful fallback is preferable.

2. The Vite 7 test passes only because of the monorepo dev environment, masking the real-world break

resolveOptionalDependency (index.ts:294) falls back to vinext's own install location (selfRequire, lines 300-303). In this repo, vite-tsconfig-paths is a devDependency of packages/vinext, so loadVite7TsconfigPathsPlugin resolves it via that fallback and the test keeps vite-tsconfig-paths on Vite 7 goes green. But a real npm consumer of vinext won't have it in vinext's node_modules (it's no longer a runtime dep), so selfRequire.resolve fails and the plugin throws. The green test does not reflect the consumer experience, and there is currently no coverage for the missing-peer case at all.

Suggested additions:

  • A test that simulates a Vite 7 project without vite-tsconfig-paths resolvable and asserts the intended behavior (graceful fallback vs. actionable error).
  • Consider stubbing out the selfRequire fallback in that test so it exercises the real consumer path.

Minor

  • The error message at index.ts:313 says "upgrade to Vite 8", but since resolveTsconfigAliases already covers simple aliases on Vite 7, that advice is misleading for the common case — most apps don't actually need this plugin.

Net: the packaging change is right, but I'd gate the Vite 7 plugin behind "aliases that actually need it" (or fall back to a warning) before merging, and add coverage for the missing-peer path that doesn't lean on vinext's own devDependency.

// that use @/*, #/*, or baseUrl imports work out of the box.
// Vite 8+ supports this natively via resolve.tsconfigPaths.
...(viteMajorVersion >= 8 ? [] : [tsconfigPaths()]),
...(viteMajorVersion >= 8 ? [] : [loadVite7TsconfigPathsPlugin(earlyBaseDir)]),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates the loader promise unconditionally for every Vite 7 app and rejects when the optional peer isn't installed. Vite's asyncFlatten awaits it during config resolution, so a missing peer throws and breaks dev/build for all Vite 7 consumers — even those with no tsconfig path aliases.

Since resolveTsconfigAliases(root) (line 1252, applied at line 2054) already materializes simple aliases into resolve.alias for all Vite versions, this plugin is only needed for aliases that fallback can't handle. Consider only requiring the peer when such aliases are present, or degrading to a warning + no-op instead of a hard throw.

const plugins = vinext({ appDir: root });

expect(findNamedPlugin(plugins, "vite-tsconfig-paths")).toBeDefined();
expect(await findNamedPlugin(plugins, "vite-tsconfig-paths")).toBeDefined();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion only passes because vite-tsconfig-paths is resolvable via vinext's own devDependency in the monorepo (the selfRequire fallback in resolveOptionalDependency). It does not exercise the real consumer scenario where the peer is absent — for an npm consumer this code path throws. Please add a test for a Vite 7 project where the peer can't be resolved, asserting the intended behavior (graceful fallback or actionable error), ideally stubbing the selfRequire fallback.

@ask-bonk

ask-bonk Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review posted successfully. Here's a summary of what I found and submitted to PR #2258:

Review Summary

The Vite 8 direction is sound — dropping vite-tsconfig-paths (and its unmaintained tsconfck transitive) as a runtime dep is a real win since Vite 8 uses native resolve.tsconfigPaths.

The concern is the Vite 7 path, which regresses harder than the PR description implies:

1. Vite 7 apps hard-fail at startup if the optional peer is missing — even with no aliases (confirms Codex P2).

  • loadVite7TsconfigPathsPlugin(earlyBaseDir) is created eagerly and unconditionally for every Vite 7 app, and rejects if the peer isn't installed. Vite's asyncFlatten awaits it during config resolution, so it throws and breaks dev/build for all Vite 7 consumers without the peer.
  • vinext already materializes simple tsconfig aliases into resolve.alias via resolveTsconfigAliases(root) for all Vite versions, so the hard failure is unnecessary for apps with no aliases or only simple @/*-style aliases.
  • getMissingDeps in deploy.ts doesn't add the peer, so generated Vite 7 deploys also hit this.

2. The Vite 7 test is misleading.

  • resolveOptionalDependency falls back to vinext's own install location, where vite-tsconfig-paths exists as a monorepo devDependency. That's why the "keeps vite-tsconfig-paths on Vite 7" test goes green — but a real npm consumer won't have it and will throw. There's no coverage for the missing-peer case.

Minor: the "upgrade to Vite 8" error message overstates the requirement since simple aliases already work on Vite 7.

I posted a batch review with a detailed summary plus 2 inline comments (on the unconditional plugin creation at index.ts:1143 and the misleading test at tsconfig-paths-vite8.test.ts:74). Recommended gating the Vite 7 plugin behind "aliases that actually need it" (or graceful fallback) and adding real missing-peer coverage before merge.

github run

@james-elicx james-elicx merged commit 6b55874 into cloudflare:main Jun 22, 2026
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants