Vite 7 support #236
Vite 7 support #236
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThe peer dependency range for "vite" in package.json was expanded to include version 7.x.x. The CI workflow updated working directories and test commands for rolldown and rollup jobs and added a new job to test Vite across multiple Node.js versions and OSes. New test packages for rolldown, rollup, and vite were added with their own package.json, source files, and tests. The "rolldown" dependency was removed from the rollup test package and added as a separate test package. An obsolete artifact stats.json file was deleted. Changes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
80-82: Range maintenance nitpick – use an upper bound instead of enumerating majors
Enumerating every major (^5 || ^6 || ^7) grows noise over time. A more future-proof expression is:- "vite": "^5.0.0 || ^6.0.0 || ^7.0.0", + "vite": ">=5 <8",This still blocks breaking Vite 8 while accepting any patch/minor in 5-7, reducing future edits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
package.json (1)
80-82: Confirm real compatibility with Vite 7 before publishing
The widened peer range advertises full support for Vite 7. That’s a contractual promise to downstream users, so please verify:
- CI matrix runs the unit/integration tests against
vite@7in addition to 5 & 6.- Manual smoke-test the plugin inside a fresh Vite 7 project to catch API-level breakages (e.g. changes in
configResolved, altered Rollup option passthrough).If fixes are needed, ship them in the same release rather than publishing a range that isn’t actually supported.
vio
force-pushed
the
add-vite-7-support
branch
2 times, most recently
from
66ab48c to
7a8420c
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
test/package/rolldown/package.json (1)
8-8: Update the test script placeholder.The test script currently outputs an error message and exits with failure. This should be updated to run actual tests once they are implemented.
Do you want me to help generate a proper test script once the test implementation is ready?
test/package/vite/vite-package.test.js (1)
37-37: Simplify path construction using path.resolve.The current path construction could be simplified and made more robust.
Apply this diff to improve path handling:
- const actual = await fs.readFile(path.join(relativeFileNameConfig.output.dir, '../artifacts/stats.json'), 'utf8'); + const actual = await fs.readFile(path.resolve(relativeFileNameConfig.output.dir, '../artifacts/stats.json'), 'utf8');.github/workflows/ci.yml (1)
196-196: Remove trailing spaces.There are trailing spaces on this line that should be removed.
Apply this diff to remove trailing spaces:
- npm install + npm install
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test/package/package-lock.jsonis excluded by!**/package-lock.jsontest/package/vite/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/ci.yml(2 hunks)artifacts/stats.json(0 hunks)package.json(3 hunks)test/package/rolldown/package.json(1 hunks)test/package/rolldown/rolldown-package.test.js(1 hunks)test/package/rollup/package.json(0 hunks)test/package/rollup/src/index.js(1 hunks)test/package/vite/index.html(1 hunks)test/package/vite/package.json(1 hunks)test/package/vite/src/index.js(1 hunks)test/package/vite/vite-package.test.js(1 hunks)test/package/vite/vite.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- artifacts/stats.json
- test/package/rollup/package.json
✅ Files skipped from review due to trivial changes (5)
- test/package/vite/src/index.js
- test/package/rolldown/rolldown-package.test.js
- test/package/vite/index.html
- test/package/vite/package.json
- test/package/vite/vite.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/package/rollup/src/index.js
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/package/vite/vite-package.test.js (2)
test/unit/fixtures/rollup-stats.ts (1)
stats(1-33)test/package/vite/vite.config.js (6)
dynamicOptions(12-21)dynamicOptions(12-21)relativeFileNameConfig(23-33)relativeFileNameConfig(23-33)absoluteFileNameConfig(35-45)absoluteFileNameConfig(35-45)
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
154-154: undefined variable "vite". available variables are "env", "github", "inputs", "job", "matrix", "needs", "runner", "secrets", "steps", "strategy", "vars"
(expression)
170-170: value "19" in "exclude" does not match in matrix "node" combinations. possible values are "22", "20", "18"
(matrix)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 196-196: trailing spaces
(trailing-spaces)
🔇 Additional comments (9)
test/package/rolldown/package.json (1)
13-14: Dependencies look appropriate for the rolldown test package.The rolldown beta version and wildcard reference to rollup-plugin-stats are suitable for testing purposes.
test/package/vite/vite-package.test.js (1)
1-7: Import statements and configuration look good.The imports are properly structured and the configuration imports match the expected usage pattern.
.github/workflows/ci.yml (7)
105-105: Working directory update looks correct.The working directory change aligns with the new package structure.
108-108: Test script update is appropriate.The updated script name matches the new package structure.
148-148: Working directory update for rolldown is correct.Consistent with the package restructuring.
151-151: Test script update for rolldown is appropriate.Matches the expected naming convention.
197-199: Vite installation and verification steps look good.The installation and version check commands are appropriate for testing Vite compatibility.
202-202: Test execution command is correct.The test script name follows the established pattern.
208-208: Dependencies updated correctly.The new vite test job is properly included in the dependencies.
| test('should output stats JSON file with explicit compilation file name', async () => { | ||
| await vite(dynamicOptions); | ||
|
|
||
| const actual = await fs.readFile(path.join(dynamicOptions.output.dir, 'stats.cjs.json'), 'utf8'); |
There was a problem hiding this comment.
Fix incorrect file extension in test.
The test expects stats.cjs.json but the configuration shows stats.commonjs.json in the relevant code snippets.
Apply this diff to fix the file extension:
- const actual = await fs.readFile(path.join(dynamicOptions.output.dir, 'stats.cjs.json'), 'utf8');
+ const actual = await fs.readFile(path.join(dynamicOptions.output.dir, 'stats.commonjs.json'), 'utf8');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const actual = await fs.readFile(path.join(dynamicOptions.output.dir, 'stats.cjs.json'), 'utf8'); | |
| const actual = await fs.readFile( | |
| path.join(dynamicOptions.output.dir, 'stats.commonjs.json'), | |
| 'utf8' | |
| ); |
🤖 Prompt for AI Agents
In test/package/vite/vite-package.test.js at line 27, the test reads the file
'stats.cjs.json' but the correct filename based on the configuration is
'stats.commonjs.json'. Update the filename in the fs.readFile call to
'stats.commonjs.json' to match the expected output and fix the test.
| beforeEach(() => { | ||
| vol.reset(); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding proper memfs setup.
While vol.reset() clears the in-memory filesystem, ensure that the file system operations in the tests are properly mocked to use memfs instead of the real filesystem.
Verify that the Vite build and file read operations are properly using memfs:
🏁 Script executed:
#!/bin/bash
# Description: Check if memfs is properly configured for file operations
# Expected: Find memfs configuration or mocking setup
rg -A 5 -B 5 "memfs|vol\." test/package/vite/Length of output: 1132
Add explicit memfs mocking for fs/promises in your Vite tests
We currently reset vol but never redirect Node’s FS API to use memfs, so calls to import fs from 'node:fs/promises' still hit the real disk. To fix:
• In test/package/vite/vite-package.test.js (at the top, before any FS operations):
-import fs from 'node:fs/promises';
import path from 'node:path';
+import { fs as memfs, vol } from 'memfs';
+import fs from 'node:fs/promises';
import { describe, test, expect, beforeEach, vi } from 'vitest';
import { build as vite } from 'vite';
import config, { dynamicOptions, relativeFileNameConfig, absoluteFileNameConfig } from './vite.config';
+// Redirect all fs/promises calls to memfs
+vi.mock('node:fs/promises', () => memfs.promises);• Keep your existing reset in beforeEach:
beforeEach(() => {
vol.reset();
});With this in place, all file reads/writes in the Vite build and tests will correctly target the in-memory filesystem.
🤖 Prompt for AI Agents
In test/package/vite/vite-package.test.js around lines 10 to 12, you currently
reset the memfs volume but do not mock the 'fs/promises' module to use memfs,
causing file operations to access the real filesystem. To fix this, add an
explicit mock for 'fs/promises' at the top of the file before any filesystem
operations, redirecting it to use memfs. Keep the existing vol.reset() call in
beforeEach to reset the in-memory filesystem between tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
194-198: Remove YAML-lint reported trailing whitespace
yamllintis failing on line 196 due to the extra blank afternpm install.
Tidy this up to keep the workflow file clean and avoid CI noise.- npm install + npm install
195-199: Consider installing the desired Vite version in one goRunning
npm installand then immediatelynpm install vite@${{ matrix.vite }} --forceperforms two full installs and leavespackage.jsonout-of-sync. A single, explicit install keeps things faster and reproducible:- npm install - npm install vite@${{ matrix.vite }} --force + npm ci --ignore-scripts + npm install vite@${{ matrix.vite }} --save-exact --no-audit --no-fund(The first line uses
npm cifor deterministic installs; adjust if lock-file is absent.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/package/vite/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/ci.yml(2 hunks)package.json(3 hunks)test/package/vite/index.html(1 hunks)test/package/vite/package.json(1 hunks)test/package/vite/src/index.js(1 hunks)test/package/vite/vite-package.test.js(1 hunks)test/package/vite/vite.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/package/vite/vite.config.js
🚧 Files skipped from review as they are similar to previous changes (5)
- test/package/vite/src/index.js
- test/package/vite/index.html
- test/package/vite/package.json
- test/package/vite/vite-package.test.js
- package.json
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 196-196: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: vite v6 / node 22 / windows-latest
- GitHub Check: vite v6 / node 18 / ubuntu-latest
- GitHub Check: vite v5 / node 18 / ubuntu-latest
- GitHub Check: vite v6 / node 20 / ubuntu-latest
- GitHub Check: vite v7 / node 20 / ubuntu-latest
- GitHub Check: vite v6 / node 22 / ubuntu-latest
- GitHub Check: vite v5 / node 20 / ubuntu-latest
- GitHub Check: vite v7 / node 22 / windows-latest
- GitHub Check: vite v5 / node 22 / windows-latest
- GitHub Check: vite v7 / node 22 / ubuntu-latest
- GitHub Check: vite v5 / node 22 / ubuntu-latest
- GitHub Check: rollup v4 / node 18 / ubuntu-latest
- GitHub Check: rollup v4 / node 20 / ubuntu-latest
- GitHub Check: rollup v4 / node 22 / windows-latest
- GitHub Check: rollup v3 / node 22 / windows-latest
- GitHub Check: rollup v3 / node 22 / ubuntu-latest
- GitHub Check: rollup v4 / node 22 / ubuntu-latest
- GitHub Check: rolldown v1 / node 22 / ubuntu-latest
- GitHub Check: rolldown v1 / node 22 / windows-latest
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
160-166: Double-check that the runner action supports Node 22All three matrix definitions include
"22":node: ["22", ...]
relative-ci/.github/actions/setup-node-npm@v1.1.0must recognise that version; otherwise the jobs will fail at setup time.
Please verify that the action (and the underlyingactions/setup-node) already ships a Node 22 manifest, or pin to an LTS version (20) until support lands.
Let me know if you want a quick verification script.Also applies to: 65-66, 118-121
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/package/vite/vite-package.test.js (1)
1-1: Duplicate comment: memfs mocking issue persists.The previous review comment about missing memfs mocking for
fs/promisesis still valid. The current code importsfs from 'node:fs/promises'but doesn't redirect it to use memfs, causing file operations to hit the real filesystem instead of the in-memory filesystem.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
196-196: Fix trailing spaces.Static analysis detected trailing spaces on this line.
Apply this diff to remove trailing spaces:
- npm install + npm install
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/package/vite/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/ci.yml(2 hunks)package.json(3 hunks)test/package/vite/index.html(1 hunks)test/package/vite/package.json(1 hunks)test/package/vite/src/index.js(1 hunks)test/package/vite/vite-package.test.js(1 hunks)test/package/vite/vite.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/package/vite/src/index.js
- test/package/vite/package.json
- package.json
- test/package/vite/vite.config.js
- test/package/vite/index.html
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/package/vite/vite-package.test.js (2)
test/unit/fixtures/rollup-stats.ts (1)
stats(1-33)test/package/vite/vite.config.js (4)
relativeFileNameConfig(25-32)relativeFileNameConfig(25-32)absoluteFileNameConfig(34-41)absoluteFileNameConfig(34-41)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 196-196: trailing spaces
(trailing-spaces)
🔇 Additional comments (5)
test/package/vite/vite-package.test.js (1)
17-17: No changes needed for output directory property
Verified thatvite.config.js’sbaseConfigincludes anoutput.dir = 'dist'. Bothconfig.output.dirandrelativeFileNameConfig.output.dircorrectly reference this property, so no updates are required..github/workflows/ci.yml (4)
105-105: Good change: Updated working directories for better isolation.The working directories have been correctly updated to point to the specific test package subdirectories, improving test isolation and organization.
Also applies to: 148-148
108-108: Good change: Updated test commands for targeted execution.The test commands now run specific package test scripts, aligning with the new test package structure and allowing for more focused testing.
Also applies to: 151-151
153-203: Well-structured Vite test job with comprehensive matrix.The new
test-package-vitejob is well-configured with appropriate matrix combinations and exclusions. The structure follows the existing patterns for rollup and rolldown test jobs.
208-208: Good addition: Updated test dependencies to include Vite testing.The
testsjob now correctly depends on all three package test jobs (rollup, rolldown, and vite), ensuring comprehensive testing before proceeding to publish.
| test('should output stats JSON file', async () => { | ||
| await vite(config); | ||
|
|
||
| const actual = await fs.readFile(path.join(__dirname, config.output.dir, 'stats.json'), 'utf8'); |
There was a problem hiding this comment.
Fix ES module compatibility issue with __dirname.
The __dirname variable is not available in ES modules. Since this file uses import statements, it's an ES module and needs an alternative approach.
Apply this diff to fix the ES module compatibility:
+import { fileURLToPath } from 'node:url';
+import { dirname } from 'node:path';
+
+const __filename = fileURLToPath(import.meta.url);
+const __dirname = dirname(__filename);Or alternatively, use a relative path approach if the test working directory is predictable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const actual = await fs.readFile(path.join(__dirname, config.output.dir, 'stats.json'), 'utf8'); | |
| import { fileURLToPath } from 'node:url'; | |
| import { dirname } from 'node:path'; | |
| const __filename = fileURLToPath(import.meta.url); | |
| const __dirname = dirname(__filename); | |
| const actual = await fs.readFile(path.join(__dirname, config.output.dir, 'stats.json'), 'utf8'); |
🤖 Prompt for AI Agents
In test/package/vite/vite-package.test.js at line 17, the use of __dirname
causes an ES module compatibility issue because __dirname is not defined in ES
modules. To fix this, replace __dirname with an alternative approach such as
using import.meta.url with the URL and path modules to derive the directory
path, or use a relative path if the test working directory is predictable.
Adjust the path.join call accordingly to ensure the correct file path is
constructed without relying on __dirname.
Summary by CodeRabbit