Conversation
🦋 Changeset detectedLatest commit: 29a0cd3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces a patch release for 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/index.ts (1)
808-811: Avoid fully silent fallback on CJS transform errors.At Line 808-Line 811, swallowing the error and returning
nullmakes SSR transform failures hard to diagnose. Consider logging a debug/warn message withidbefore fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 808 - 811, The catch block in transformCjsToEsm is currently swallowing errors and returning null; update the try/catch around the call to transformCjsToEsm(source, id) to catch the error as a variable (e.g., err) and emit a suitable log (debug/warn) that includes the module id and the error message/stack before returning null so SSR transform failures are visible; reference the transformCjsToEsm call and the id parameter when adding the log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/fixtures/isomorphic-commonjs-module-exports/src/index.js`:
- Around line 4-9: The request handler only responds when req.url === "/"
leaving other requests hanging; add a non-root fallback in the same handler to
set an appropriate status and body (e.g., 404 or 405) and end the response when
req.url !== "/", by calling res.statusCode = 404 (or other),
res.setHeader("Content-Type", ...), and res.end(...) so all paths (including the
branch invoking template.render(...)) always terminate the response.
In `@src/cjs-to-esm.ts`:
- Around line 5-7: REQUIRE_RE and REQUIRE_TEST_RE incorrectly match require()
occurrences inside comments; replace the regex-based detection with a
syntax-aware scan: parse the source (using the existing es-module-lexer or a JS
parser like acorn used elsewhere) and find CallExpression nodes whose callee is
an Identifier named "require" (and ensure they are not inside comments or string
literals), then rewrite only those AST-located ranges instead of using
REQUIRE_RE/REQUIRE_TEST_RE; update any code that references
REQUIRE_RE/REQUIRE_TEST_RE (and keep EXPORTS_RE for exports detection if still
needed) to use the new parser-based detection logic so commented-out require
calls are ignored.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 808-811: The catch block in transformCjsToEsm is currently
swallowing errors and returning null; update the try/catch around the call to
transformCjsToEsm(source, id) to catch the error as a variable (e.g., err) and
emit a suitable log (debug/warn) that includes the module id and the error
message/stack before returning null so SSR transform failures are visible;
reference the transformCjsToEsm call and the id parameter when adding the log.
🪄 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: 63622409-2bcc-443d-a19d-dbd0b62e018c
⛔ Files ignored due to path filters (7)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**src/__tests__/fixtures/isomorphic-commonjs-module-exports/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-commonjs-module-exports/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-commonjs-module-exports/node_modules/dep/index.jsis excluded by!**/node_modules/**and included by**src/__tests__/fixtures/isomorphic-commonjs-module-exports/node_modules/dep/package.jsonis excluded by!**/node_modules/**and included by**src/__tests__/fixtures/isomorphic-commonjs-module-exports/node_modules/transient-dep/index.jsis excluded by!**/node_modules/**and included by**src/__tests__/fixtures/isomorphic-commonjs-module-exports/node_modules/transient-dep/package.jsonis excluded by!**/node_modules/**and included by**
📒 Files selected for processing (11)
.changeset/nice-tires-stare.mdpackage.jsonsrc/__tests__/fixtures/isomorphic-commonjs-module-exports/dev-server.mjssrc/__tests__/fixtures/isomorphic-commonjs-module-exports/package.jsonsrc/__tests__/fixtures/isomorphic-commonjs-module-exports/server.mjssrc/__tests__/fixtures/isomorphic-commonjs-module-exports/src/index.jssrc/__tests__/fixtures/isomorphic-commonjs-module-exports/src/template.markosrc/__tests__/fixtures/isomorphic-commonjs-module-exports/test.config.tssrc/__tests__/fixtures/isomorphic-commonjs-module-exports/vite.config.mtssrc/cjs-to-esm.tssrc/index.ts
| if (req.url === "/") { | ||
| res.statusCode = 200; | ||
| res.setHeader("Content-Type", "text/html; charset=utf-8"); | ||
| template.render({}, res); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a non-root fallback to avoid hanging requests.
On Line 4, only "/" is handled. Other URLs currently get no status/body, which can leave the connection open.
Suggested fix
export function handler(req, res) {
- if (req.url === "/") {
- res.statusCode = 200;
- res.setHeader("Content-Type", "text/html; charset=utf-8");
- template.render({}, res);
- }
+ if (req.url !== "/") {
+ res.statusCode = 404;
+ res.end("Not Found");
+ return;
+ }
+
+ res.statusCode = 200;
+ res.setHeader("Content-Type", "text/html; charset=utf-8");
+ template.render({}, res);
}📝 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.
| if (req.url === "/") { | |
| res.statusCode = 200; | |
| res.setHeader("Content-Type", "text/html; charset=utf-8"); | |
| template.render({}, res); | |
| } | |
| } | |
| if (req.url !== "/") { | |
| res.statusCode = 404; | |
| res.end("Not Found"); | |
| return; | |
| } | |
| res.statusCode = 200; | |
| res.setHeader("Content-Type", "text/html; charset=utf-8"); | |
| template.render({}, res); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/fixtures/isomorphic-commonjs-module-exports/src/index.js`
around lines 4 - 9, The request handler only responds when req.url === "/"
leaving other requests hanging; add a non-root fallback in the same handler to
set an appropriate status and body (e.g., 404 or 405) and end the response when
req.url !== "/", by calling res.statusCode = 404 (or other),
res.setHeader("Content-Type", ...), and res.end(...) so all paths (including the
branch invoking template.render(...)) always terminate the response.
| const REQUIRE_RE = /([^.\w$]|^)require\s*\(\s*(['"])(.*?)\2\s*\)/g; | ||
| const REQUIRE_TEST_RE = /([^.\w$]|^)require\s*\(\s*['"]/; | ||
| const EXPORTS_RE = /\b(module\.exports|exports)\b/; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify regex false-positives against non-code contexts (read-only demonstration).
python - <<'PY'
import re
REQUIRE_RE = re.compile(r"([^.\w$]|^)require\s*\(\s*(['\"])(.*?)\2\s*\)")
samples = {
"string_literal": 'const s = "require(\\"fs\\")"; module.exports = {};',
"line_comment": "// require('path')\nmodule.exports = 1;",
"block_comment": "/* require('http') */\nmodule.exports = 2;",
"real_call": "const fs = require('fs'); module.exports = fs;"
}
for name, code in samples.items():
matches = [m.group(0) for m in REQUIRE_RE.finditer(code)]
print(f"{name}: {matches}")
PYRepository: marko-js/vite
Length of output: 176
🏁 Script executed:
# First, let's check the file structure and read the relevant sections
git ls-files src/cjs-to-esm.ts
# Read the first section around lines 5-7
sed -n '1,20p' src/cjs-to-esm.ts
# Read the second section around lines 57-72
sed -n '50,75p' src/cjs-to-esm.ts
# Get total file size for context
wc -l src/cjs-to-esm.tsRepository: marko-js/vite
Length of output: 1640
🏁 Script executed:
# Get more context around the regex usage - lines 20-80 to see function signature and full usage
sed -n '20,80p' src/cjs-to-esm.tsRepository: marko-js/vite
Length of output: 1527
🏁 Script executed:
# Get the rest of the function and lines 57-72 specifically
sed -n '52,100p' src/cjs-to-esm.tsRepository: marko-js/vite
Length of output: 1453
Regex-based require() rewriting matches inside comments, producing incorrect transformations.
The REQUIRE_RE pattern operates on raw source text without syntax awareness. While string literals are generally safe (quotes prevent accidental matches), commented-out code like // require('fs') or /* require('http') */ will be matched and rewritten. This corrupts comments and can produce invalid output.
The codebase already uses lexer-based parsing (via cjs-module-lexer for exports and es-module-lexer for module detection), so switching the require-call matching to a similar syntax-aware approach is feasible and recommended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cjs-to-esm.ts` around lines 5 - 7, REQUIRE_RE and REQUIRE_TEST_RE
incorrectly match require() occurrences inside comments; replace the regex-based
detection with a syntax-aware scan: parse the source (using the existing
es-module-lexer or a JS parser like acorn used elsewhere) and find
CallExpression nodes whose callee is an Identifier named "require" (and ensure
they are not inside comments or string literals), then rewrite only those
AST-located ranges instead of using REQUIRE_RE/REQUIRE_TEST_RE; update any code
that references REQUIRE_RE/REQUIRE_TEST_RE (and keep EXPORTS_RE for exports
detection if still needed) to use the new parser-based detection logic so
commented-out require calls are ignored.
1 participant
No description provided.