feat: support disabling Vite+'s Node.js management with node-manager: false - #115
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for explicitly disabling Vite+ Node.js version management via the action input node-version: off, aimed at users who manage Node.js through other tooling.
Changes:
- Update main action flow to treat
node-version: offas a request to runvp env off. - Update
action.ymlinput documentation to mention theoffoption.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/index.ts |
Adds nodeVersion !== "off" guard and an else if branch to run vp env off. |
action.yml |
Updates node-version input description to mention off. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3376754c01
ℹ️ 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".
|
turns out I forgot to push my packed dist/index.mjs |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
action.yml:25
- The
node-versioninput description still claims it "Defaults to Node.js latest LTS version", but there is nodefault:for this input and the action only manages Node whennode-version(ornode-version-file) is provided. Since this PR updates the description, it should accurately document the three states: unset (no Node management unlessnode-version-fileis set), a version (runsvp env use), andoff(runsvp env off).
description: "Node.js version to install via `vp env use`. Defaults to Node.js latest LTS version. Pass `off` to disable."
|
waiting for merge ig |
|
@KTrain5169 Could you try this environment variable? It should be able to disable the Node.js version manager. - uses: voidzero-dev/setup-vp@v1
env:
VP_NODE_MANAGER: "no" |
will check later |
|
@fengmk2 the env variable doesn't seem to work |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
action.yml:25
- The
node-versioninput description still claims it defaults to the latest LTS, but there is nodefault:for this input and the action only runsvp env usewhen a value is provided. This is misleading, especially now thatoffis a supported explicit value. Reword to document the three states: unset (no Node management), version (env use), andoff(env off).
description: "Node.js version to install via `vp env use`. Defaults to Node.js latest LTS version. Pass `off` to disable."
src/index.ts:38
"off"is now a reserved sentinel value for thenode-versioninput, but this logic also triggers whennodeVersioncomes fromnode-version-file. If a version file accidentally containsoff, the action will runvp env off(or silently skip Node setup) instead of failing fast, which can mask a bad version file. Gate thevp env offbranch on the explicitnode-versioninput, and consider throwing ifnode-version-fileresolves to the reserved value.
info(`Setting up Node.js ${nodeVersion} via vp env use...`);
await exec("vp", ["env", "use", nodeVersion]);
} else if (nodeVersion === "off") {
info("Disabling Node.js version management with vp env off...");
await exec("vp", ["env", "off"]);
|
@KTrain5169 I will take over this pr and try to improve it in all ci envs. |
…ith `node-version: off`
…on GitHub, GitLab, and Azure Replace the node-version: off sentinel with a tri-state node-manager input, wired through all three runtimes. Unset keeps the installer's auto-detection (enabled on CI). false passes VP_NODE_MANAGER=no to the install script (skips node/npm/npx/corepack shims) and runs vp env off after install so vp commands prefer the system Node.js. true force-enables the managed Node.js. node-manager: false fails fast when combined with node-version or node-version-file. The parser accepts YAML 1.2 boolean forms because Azure serializes booleans passed to string parameters as True/False.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (3)
action.yml:32
- The
node-versioninput description currently implies the action will runvp env useby default ("Defaults to Node.js latest LTS"), butsrc/index.tsonly runsvp env usewhennode-version/node-version-fileis provided. This makes the input docs misleading, especially now thatnode-managercontrols whether Vite+'s own manager is used. Consider rewordingnode-versionto describe the conditional behavior (and optionally mention hownode-managerinteracts).
node-manager:
description: "Control Vite+'s Node.js version manager. When unset, the Vite+ installer decides (enabled on CI). Set to `false` to keep the Node.js already on the runner (e.g. from actions/setup-node or the runner image): shim creation is skipped and vp commands prefer the system Node.js. Set to `true` to force-enable the managed Node.js. Cannot be `false` together with node-version or node-version-file."
required: false
src/inputs.ts:13
- The PR title/description calls out disabling Node management via
node-version: off, but the implementation introduces a separatenode-managerinput and does not recognizenode-version=off(no matches in the codebase). This mismatch may confuse users following the PR title. Consider either supportingnode-version: offas an alias fornode-manager: false, or updating the PR title/docs to consistently refer tonode-manager: false.
const nodeVersion = getInput("node-version") || undefined;
const nodeVersionFile = getInput("node-version-file") || undefined;
const nodeManager = parseNodeManager(getInput("node-manager"));
if (nodeManager === false && (nodeVersion || nodeVersionFile)) {
throw new Error(
README.md:63
- This section documents opting out via
node-manager: false, while the PR title mentionsnode-version: off. Ifnode-version: offis no longer the intended UX, consider updating the PR title (and any related docs) to avoid conflicting guidance; otherwise, document/support thenode-version: offalias explicitly.
### Keep the Runner's Node.js
The Vite+ installer enables its own Node.js version manager on CI. When
Node.js is managed elsewhere (`actions/setup-node`, Flox, mise, or the runner
image), disable it so `vp` and its shims use that Node.js:
node-version: off…ger-off The Vite+ installer always downloads a managed Node.js LTS into $VP_HOME/js_runtime to install its own production dependencies with a deterministic runtime, independent of VP_NODE_MANAGER, so asserting an empty js_runtime cache was wrong. Assert observable behavior instead: vp exec node --version must match the runner's preinstalled Node.js.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (3)
README.md:71
- This example uses
voidzero-dev/setup-vp@v1.16.1, butnode-manageris introduced by this PR; users following the docs on v1.16.1 won't have that input available. Update the tag in this snippet to a version that includesnode-manager(or use@v1).
- uses: voidzero-dev/setup-vp@v1.16.1
gitlab/setup-vp.yml:18
node-manageris wired through this template, but the defaultsetup-refstill points atv1.16.1. That ref determines whichbootstrap.sh/runtime bundle is downloaded; with the older default,node-managerwon't be applied unless users also overridesetup-ref. Bump the defaultsetup-refto the release that includes this feature.
node-manager:
description: 'Control Vite+''s Node.js version manager. This is a string input: empty (default) lets the Vite+ installer decide (enabled on CI); "false" keeps the runner image''s Node.js (skips shim creation and makes vp commands prefer the system Node.js); "true" force-enables the managed Node.js.'
default: ""
azure/setup-vp.yml:28
- This template adds the
nodeManagerparameter and exportsSETUP_VP_NODE_MANAGER, butsetupRefstill defaults tov1.16.1(the ref used to downloadazure/bootstrap.*anddist/azure/index.mjs). With that older default,nodeManagerwon't take effect unless users also bumpsetupRef. Update the default to the version that contains this feature.
- name: nodeManager
type: string
default: ""
|
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". |
Bump the version to v1.17.0 in package.json, the README examples, and the setup-ref / setupRef defaults in the GitLab and Azure templates. Changes since v1.16.1: - feat: support disabling Vite+'s Node.js management with node-manager: false (#115) - fix: retry sfw vp install when sfw misreports vp as not found on Windows (#120) - chore(deps): update vite-plus to v0.2.8 (#119) - ci: re-run bundle rebuild when Renovate pushes to a labeled PR (#122) - chore: align Renovate release age for the whole vite-plus group (#121) After merge, tag the merge commit as v1.17.0 and push the tag.
Adds a tri-state
node-managerinput to GitHub, GitLab, and Azure. Unset keeps the installer's auto-detection (enabled on CI).falsekeeps the Node.js already on the runner: the install script skips node/npm/npx/corepack shims (VP_NODE_MANAGER=no) and the action runsvp env offso vp commands prefer the system Node.js.trueforce-enables the managed Node.js.node-manager: falsefails fast when combined withnode-versionornode-version-file. Supersedes the earliernode-version: offapproach, which broke under YAML 1.1 parsers (GitLab) and mixed a mode into a version input.Usage:
Closes the request from Discord for Flox and other external Node.js managers.