feat(build): add vite-plus support#7014
Open
TheAlexLichter wants to merge 5 commits intonetlify:mainfrom
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Vite+ support across the build system: introduces a new VitePlus build-tool class that detects Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/build-info/src/build-systems/vite-plus.test.ts (3)
55-55: Add explicit assertion before non-null usage.The
vitePlus!non-null assertion will produce a confusing runtime error if detection fails. Adding an explicit assertion improves debuggability.♻️ Suggested improvement
const vitePlus = detected.find((b) => b.name === 'Vite+') + expect(vitePlus).toBeDefined() const commands = await vitePlus!.getCommands!('')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/build-info/src/build-systems/vite-plus.test.ts` at line 55, The test currently dereferences vitePlus and its getCommands via non-null assertions (vitePlus! and getCommands!), which will throw a confusing runtime error if detection failed; instead, add an explicit assertion that vitePlus is defined (e.g., assert or expect(vitePlus).toBeDefined()) and that vitePlus.getCommands is a function before calling getCommands(''), then call vitePlus.getCommands('') without the non-null assertions so failures are reported clearly; reference the vitePlus variable and the getCommands method in your assertion checks.
17-18: Test assertion is fragile—relying on array index.Using
detected[0]assumesVitePlusis the only or first detected build system. If another build system (e.g., a package manager) also detects in the same mock project, this test could fail or pass incorrectly.Consider filtering by name like the absence test does:
♻️ Suggested improvement
- expect(detected[0]?.name).toBe('Vite+') - expect(detected[0]?.version).toBe('^1.0.0') + const vitePlus = detected.find((b) => b.name === 'Vite+') + expect(vitePlus).toBeDefined() + expect(vitePlus?.name).toBe('Vite+') + expect(vitePlus?.version).toBe('^1.0.0')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/build-info/src/build-systems/vite-plus.test.ts` around lines 17 - 18, The assertions in vite-plus.test.ts are brittle because they reference detected[0]; update the test to locate the Vite+ entry by filtering or finding by name (e.g., use detected.find(d => d?.name === 'Vite+') ) and then assert its version equals '^1.0.0' instead of assuming index 0; change both name and version checks to use the found item and assert it exists before checking version.
27-28: Same fragile assumption here.Apply the same fix as suggested for lines 17-18.
♻️ Suggested improvement
- expect(detected[0]?.name).toBe('Vite+') - expect(detected[0]?.version).toBe('^2.0.0') + const vitePlus = detected.find((b) => b.name === 'Vite+') + expect(vitePlus).toBeDefined() + expect(vitePlus?.name).toBe('Vite+') + expect(vitePlus?.version).toBe('^2.0.0')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/build-info/src/build-systems/vite-plus.test.ts` around lines 27 - 28, The test currently assumes the target result is at detected[0], which is fragile; instead locate the entry by name and assert its version. Update the assertions in vite-plus.test.ts to find the element in detected with name 'Vite+' (e.g., const vite = detected.find(d => d?.name === 'Vite+')) and then assert that vite is defined and that vite.version equals '^2.0.0' rather than accessing detected[0]?.version; use the same pattern you applied for the earlier lines 17-18 to make the test robust.packages/build/src/plugins_core/vite_plus_setup/index.test.ts (1)
95-127: Consider adding failure scenario tests.The
installVitePlusClitests only cover the success path. Consider adding a test for whenexecafails (e.g., network error, invalid script) to ensure proper error propagation.Additionally, the
conditionand maincoreStepfunctions are not directly tested—only their helper functions. Consider adding integration tests for these entry points.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/build/src/plugins_core/vite_plus_setup/index.test.ts` around lines 95 - 127, Add failing-path tests that mock execa (mockedExeca) to reject (simulate network/script error) and assert that installVitePlusCli re-throws or rejects with the same error so failures are propagated; use the same call shape as existing tests but have mockedExeca.mockRejectedValueOnce(...) and expect(await expect(installVitePlusCli(...)).rejects.toThrow(...)). Also add integration tests that call condition and coreStep directly (or via the module entry) to verify their behavior end-to-end: for condition verify returned boolean for different env/inputs, and for coreStep mock dependencies (execa, filesystem, logger) and assert coreStep invokes installVitePlusCli and handles errors/outputs as expected. Ensure you reference installVitePlusCli, condition, coreStep and mockedExeca in the new tests so they target the right functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/build-info/src/build-systems/vite-plus.ts`:
- Around line 23-35: getCommands currently calls
this.project.fs.readJSON(this.project.resolveFromPackage(...)) without error
handling, so a missing or malformed package.json will throw; wrap the readJSON
call in a try/catch (or check existence via resolveFromPackage) inside
getCommands, catch errors from readJSON and return an empty array on failure,
and keep the same mapping logic that uses isNpmDevScript/isNpmBuildScript to
produce Command objects; refer to symbols getCommands, this.project.fs.readJSON,
this.project.resolveFromPackage, isNpmDevScript, and isNpmBuildScript when
making the change so the error handling surrounds the JSON read and
early-returns [] on error.
In `@packages/build/src/plugins_core/vite_plus_setup/index.ts`:
- Around line 50-70: The condition function (condition) calls getPackageJson
twice and can throw, so wrap each getPackageJson call (both the top-level
getPackageJson(buildDir) and the workspace getPackageJson(join(buildDir,
packagePath))) in try/catch blocks and handle errors by returning false (or
otherwise treating as "no Vite Plus package"); ensure you still call
hasVitePlusPackage on successful parses and optionally log or debug the caught
error for visibility; this prevents exceptions from aborting the build during
the step check.
---
Nitpick comments:
In `@packages/build-info/src/build-systems/vite-plus.test.ts`:
- Line 55: The test currently dereferences vitePlus and its getCommands via
non-null assertions (vitePlus! and getCommands!), which will throw a confusing
runtime error if detection failed; instead, add an explicit assertion that
vitePlus is defined (e.g., assert or expect(vitePlus).toBeDefined()) and that
vitePlus.getCommands is a function before calling getCommands(''), then call
vitePlus.getCommands('') without the non-null assertions so failures are
reported clearly; reference the vitePlus variable and the getCommands method in
your assertion checks.
- Around line 17-18: The assertions in vite-plus.test.ts are brittle because
they reference detected[0]; update the test to locate the Vite+ entry by
filtering or finding by name (e.g., use detected.find(d => d?.name === 'Vite+')
) and then assert its version equals '^1.0.0' instead of assuming index 0;
change both name and version checks to use the found item and assert it exists
before checking version.
- Around line 27-28: The test currently assumes the target result is at
detected[0], which is fragile; instead locate the entry by name and assert its
version. Update the assertions in vite-plus.test.ts to find the element in
detected with name 'Vite+' (e.g., const vite = detected.find(d => d?.name ===
'Vite+')) and then assert that vite is defined and that vite.version equals
'^2.0.0' rather than accessing detected[0]?.version; use the same pattern you
applied for the earlier lines 17-18 to make the test robust.
In `@packages/build/src/plugins_core/vite_plus_setup/index.test.ts`:
- Around line 95-127: Add failing-path tests that mock execa (mockedExeca) to
reject (simulate network/script error) and assert that installVitePlusCli
re-throws or rejects with the same error so failures are propagated; use the
same call shape as existing tests but have
mockedExeca.mockRejectedValueOnce(...) and expect(await
expect(installVitePlusCli(...)).rejects.toThrow(...)). Also add integration
tests that call condition and coreStep directly (or via the module entry) to
verify their behavior end-to-end: for condition verify returned boolean for
different env/inputs, and for coreStep mock dependencies (execa, filesystem,
logger) and assert coreStep invokes installVitePlusCli and handles
errors/outputs as expected. Ensure you reference installVitePlusCli, condition,
coreStep and mockedExeca in the new tests so they target the right functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c054ef95-5493-4947-bc44-0fd0358efd9d
📒 Files selected for processing (6)
packages/build-info/src/build-systems/index.tspackages/build-info/src/build-systems/vite-plus.test.tspackages/build-info/src/build-systems/vite-plus.tspackages/build/src/plugins_core/vite_plus_setup/index.test.tspackages/build/src/plugins_core/vite_plus_setup/index.tspackages/build/src/steps/get.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/build-info/src/build-systems/vite-plus.test.ts (2)
17-17: Prefer asserting by build-systemidinstead of displayname.Using
namemakes tests fragile to cosmetic renames.idis the stable contract key.Suggested change
- const vitePlus = detected.find((b) => b.name === 'Vite+') + const vitePlus = detected.find((b) => b.id === 'vite-plus')Also applies to: 28-28, 39-39, 55-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/build-info/src/build-systems/vite-plus.test.ts` at line 17, Tests currently locate the Vite+ entry by matching its display name (detected.find((b) => b.name === 'Vite+')), which is brittle; change these assertions to use the stable build-system id instead (e.g., detected.find((b) => b.id === 'vite-plus')). Update the occurrences that create the vitePlus variable and any other similar finds/assertions in this file to match on b.id === 'vite-plus' rather than b.name, and adjust any related expectations that relied on the name lookup accordingly.
11-31: Add a precedence test when both dependency blocks definevite-plus.Current tests validate each source independently, but not the conflict case. A small extra test would lock in the intended precedence (
devDependenciesoverdependencies) and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/build-info/src/build-systems/vite-plus.test.ts` around lines 11 - 31, Add a new unit test in the same test file to assert precedence when both dependency blocks include vite-plus: create a mock package.json that has 'vite-plus' in both devDependencies and dependencies with different versions, call new Project(fs, cwd).detectBuildSystem(), then verify the detected entry for name 'Vite+' exists and that its version equals the devDependencies version (ensuring devDependencies takes precedence over dependencies); reference the existing tests 'detects Vite+ when vite-plus is in devDependencies' and 'detects Vite+ when vite-plus is in dependencies' as templates and use Project.detectBuildSystem and the lookup detected.find((b) => b.name === 'Vite+').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/build/src/plugins_core/vite_plus_setup/index.ts`:
- Around line 20-23: The getVitePlusVersion function currently calls
getPackageJson(buildDir) without guarding for read errors which can crash
coreStep even when a workspace package contains NPM_PACKAGE_NAME; wrap the root
getPackageJson(buildDir) call in a try/catch (or otherwise handle its failure)
so a missing/unreadable root package.json falls back to scanning workspace
packageJsons for NPM_PACKAGE_NAME, and ensure the code checks for a defined
packageJson before accessing devDependencies/dependencies; reference
getVitePlusVersion, getPackageJson, and NPM_PACKAGE_NAME when making the change.
- Around line 42-45: The execa invocation that runs bash with a curl installer
(the call using execa('bash', ['-c', `curl -fsSL --max-time 120 ${INSTALL_URL} |
bash`], { env: { ...process.env, VP_VERSION: version, VITE_PLUS_VERSION: version
}, stdio: 'pipe' })) should enable pipefail so curl failures are caught; change
the bash argument array to include the shell option for pipefail (i.e., pass
['-o', 'pipefail', '-c', ...] instead of just ['-c', ...]) and update the
corresponding test expectation in the vite_plus_setup test to expect ['-o',
'pipefail', '-c', ...]; keep the env and stdio settings unchanged.
- Around line 90-91: The code currently builds newPath using a hard-coded ':'
and may append a trailing delimiter when process.env.PATH is undefined; update
the PATH prepend logic to use Node's path.delimiter and only add the existing
PATH segment if process.env.PATH is non-empty: compute delimiter =
require('path').delimiter (or import path) and set process.env.PATH =
vitePlusBinDir + (process.env.PATH ? delimiter + process.env.PATH : '') so
newPath (and assignment to process.env.PATH) is cross-platform and avoids
trailing delimiters.
---
Nitpick comments:
In `@packages/build-info/src/build-systems/vite-plus.test.ts`:
- Line 17: Tests currently locate the Vite+ entry by matching its display name
(detected.find((b) => b.name === 'Vite+')), which is brittle; change these
assertions to use the stable build-system id instead (e.g., detected.find((b) =>
b.id === 'vite-plus')). Update the occurrences that create the vitePlus variable
and any other similar finds/assertions in this file to match on b.id ===
'vite-plus' rather than b.name, and adjust any related expectations that relied
on the name lookup accordingly.
- Around line 11-31: Add a new unit test in the same test file to assert
precedence when both dependency blocks include vite-plus: create a mock
package.json that has 'vite-plus' in both devDependencies and dependencies with
different versions, call new Project(fs, cwd).detectBuildSystem(), then verify
the detected entry for name 'Vite+' exists and that its version equals the
devDependencies version (ensuring devDependencies takes precedence over
dependencies); reference the existing tests 'detects Vite+ when vite-plus is in
devDependencies' and 'detects Vite+ when vite-plus is in dependencies' as
templates and use Project.detectBuildSystem and the lookup detected.find((b) =>
b.name === 'Vite+').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18cf182a-ba0f-496b-96b8-e25d2d7200ce
📒 Files selected for processing (4)
packages/build-info/src/build-systems/vite-plus.test.tspackages/build-info/src/build-systems/vite-plus.tspackages/build/src/plugins_core/vite_plus_setup/index.test.tspackages/build/src/plugins_core/vite_plus_setup/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/build/src/plugins_core/vite_plus_setup/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/build-info/src/build-systems/vite-plus.ts
Comment on lines
+20
to
+23
| export const getVitePlusVersion = async (buildDir: string, packagePath?: string): Promise<string> => { | ||
| const { packageJson } = await getPackageJson(buildDir) | ||
| const version = packageJson.devDependencies?.[NPM_PACKAGE_NAME] ?? packageJson.dependencies?.[NPM_PACKAGE_NAME] | ||
|
|
Contributor
There was a problem hiding this comment.
getVitePlusVersion can fail after condition already passed.
At Line 21, root getPackageJson(buildDir) is not guarded. If root package.json is unreadable but workspace has vite-plus, condition returns true and coreStep still crashes during version resolution.
Proposed fix
export const getVitePlusVersion = async (buildDir: string, packagePath?: string): Promise<string> => {
- const { packageJson } = await getPackageJson(buildDir)
- const version = packageJson.devDependencies?.[NPM_PACKAGE_NAME] ?? packageJson.dependencies?.[NPM_PACKAGE_NAME]
-
- if (version) {
- return version
- }
+ try {
+ const { packageJson } = await getPackageJson(buildDir)
+ const version = packageJson.devDependencies?.[NPM_PACKAGE_NAME] ?? packageJson.dependencies?.[NPM_PACKAGE_NAME]
+ if (version) {
+ return version
+ }
+ } catch {
+ // noop — root package.json missing or invalid
+ }
if (packagePath) {
- const { packageJson: workspacePackageJson } = await getPackageJson(join(buildDir, packagePath))
- return (
- workspacePackageJson.devDependencies?.[NPM_PACKAGE_NAME] ??
- workspacePackageJson.dependencies?.[NPM_PACKAGE_NAME] ??
- 'latest'
- )
+ try {
+ const { packageJson: workspacePackageJson } = await getPackageJson(join(buildDir, packagePath))
+ return (
+ workspacePackageJson.devDependencies?.[NPM_PACKAGE_NAME] ??
+ workspacePackageJson.dependencies?.[NPM_PACKAGE_NAME] ??
+ 'latest'
+ )
+ } catch {
+ // noop — workspace package.json missing or invalid
+ }
}
return 'latest'
}Also applies to: 28-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/build/src/plugins_core/vite_plus_setup/index.ts` around lines 20 -
23, The getVitePlusVersion function currently calls getPackageJson(buildDir)
without guarding for read errors which can crash coreStep even when a workspace
package contains NPM_PACKAGE_NAME; wrap the root getPackageJson(buildDir) call
in a try/catch (or otherwise handle its failure) so a missing/unreadable root
package.json falls back to scanning workspace packageJsons for NPM_PACKAGE_NAME,
and ensure the code checks for a defined packageJson before accessing
devDependencies/dependencies; reference getVitePlusVersion, getPackageJson, and
NPM_PACKAGE_NAME when making the change.
Comment on lines
+90
to
+91
| const newPath = `${vitePlusBinDir}:${process.env.PATH ?? ''}` | ||
| process.env.PATH = newPath |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/build/src/plugins_core/vite_plus_setup/index.ts | head -100Repository: netlify/build
Length of output: 3791
🏁 Script executed:
find . -type f -name "*.test.*" -o -name "*.spec.*" | grep -i "vite" | head -20Repository: netlify/build
Length of output: 233
🏁 Script executed:
cat -n packages/build/src/plugins_core/vite_plus_setup/index.test.tsRepository: netlify/build
Length of output: 5086
Use path.delimiter and avoid trailing delimiters when PATH is unset.
The hard-coded : delimiter is not cross-platform (Windows uses ;), and ${process.env.PATH ?? ''} creates a trailing delimiter when PATH is unset, resulting in an empty PATH segment. Prepend to PATH conditionally using path.delimiter:
Fix
-import { join } from 'node:path'
+import { delimiter, join } from 'node:path'
...
- const newPath = `${vitePlusBinDir}:${process.env.PATH ?? ''}`
+ const newPath = process.env.PATH ? `${vitePlusBinDir}${delimiter}${process.env.PATH}` : vitePlusBinDir🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/build/src/plugins_core/vite_plus_setup/index.ts` around lines 90 -
91, The code currently builds newPath using a hard-coded ':' and may append a
trailing delimiter when process.env.PATH is undefined; update the PATH prepend
logic to use Node's path.delimiter and only add the existing PATH segment if
process.env.PATH is non-empty: compute delimiter = require('path').delimiter (or
import path) and set process.env.PATH = vitePlusBinDir + (process.env.PATH ?
delimiter + process.env.PATH : '') so newPath (and assignment to
process.env.PATH) is cross-platform and avoids trailing delimiters.
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
1 participant
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.
Summary
Fixes #7013
This PR tries to add Vite+ support to the build image.
Disclosure: The PR was written together with an LLM.
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)