fix(config): make vite-tsconfig-paths an optional peer#2258
Conversation
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
There was a problem hiding this comment.
💡 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)]), |
There was a problem hiding this comment.
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 👍 / 👎.
|
/bigbonk review for issues |
There was a problem hiding this comment.
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-pathsresolvable and asserts the intended behavior (graceful fallback vs. actionable error). - Consider stubbing out the
selfRequirefallback 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
resolveTsconfigAliasesalready 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)]), |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
|
Review posted successfully. Here's a summary of what I found and submitted to PR #2258: Review SummaryThe Vite 8 direction is sound — dropping 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).
2. The Vite 7 test is misleading.
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. |
Summary
vite-tsconfig-pathsout of vinext runtime dependencies and declare it as an optional peerresolve.tsconfigPathsvite-tsconfig-pathsas a dev dependency for repository tests and update the Vite 7/8 coverageWhy
Vite 8 no longer needs
vite-tsconfig-paths, and keeping it as a runtime dependency installs its unmaintainedtsconfcktransitive dependency for users who do not need the Vite 7 fallback.Impact
Vite 8 consumers no longer get
vite-tsconfig-pathsortsconfckfrom 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-scriptsvp test run tests/tsconfig-paths-vite8.test.tsvp check packages/vinext/src/index.ts packages/vinext/src/deploy.ts tests/tsconfig-paths-vite8.test.tsvp run vinext#build