fix: revert cjs transform for dev ssr deps#274
Merged
Conversation
🦋 Changeset detectedLatest commit: 77db41b 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 change replaces the CommonJS-to-ESM transformation implementation to use 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 6
🧹 Nitpick comments (1)
src/cjs-to-esm.ts (1)
167-177: Dedup map uses inconsistent keys, so dedup never triggers.Line [167] reads by specifier string, but Line [176] writes by AST node object. This guarantees duplicate entries for repeated
require("same-module").Suggested fix
- const specs = new Map(); + const specs = new Map<string, { id: string; specifier: string }>(); ... - let spec = specs.get(specifier.value); + let spec = specs.get(specifier.value); if (!spec) { let id = `$cjs$${specifier.value.replace(/[^\w_$]+/g, "_")}`; const count = (ns.get(id) || 0) + 1; ns.set(id, count); if (count > 1) { id += count; } spec = { id, specifier: specifier.value }; - specs.set(specifier, spec); + specs.set(specifier.value, spec); }🤖 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 167 - 177, The dedup map bug is caused by storing the spec under the AST node object while reading by specifier.value, preventing duplicates from being found; update the assignment to use the same string key as the lookup (use specs.set(specifier.value, spec) instead of specs.set(specifier, spec)) so reads via specs.get(specifier.value) and writes are consistent (leave id generation and ns/count logic unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/spicy-zebras-buy.md:
- Line 5: The changeset note contains a package scope typo: replace the
incorrect package name "@chialabs/estransform" with the correct
"@chialab/estransform" in the .changeset/spicy-zebras-buy.md file so the
dependency reference is accurate (look for the string "@chialabs/estransform" in
the file and update it to "@chialab/estransform").
In `@src/__tests__/cjs-to-esm.test.ts`:
- Around line 53-60: The test title is misleading: it says "ignores require()
inside arrow functions" but the assertions expect the require to be rewritten;
update the test name to reflect that behavior (e.g., "rewrites require() inside
arrow functions") so it matches the assertions around transformCjsToEsm, the
local variable source, and the result.code check for 'from \"dep\"'. Ensure you
only change the test description string in the it(...) call.
In `@src/__tests__/fixtures/isomorphic-commonjs-transpiled-default/src/index.js`:
- Around line 3-9: The handler function currently only handles req.url === "/"
and returns for all other requests, leaving the response open; update the
handler (export function handler) to explicitly handle non-root requests by
setting an appropriate status (e.g., 404), writing a response body or calling
res.end(), or delegating to a next/middleware handler if available, so that for
any req.url other than "/" you always end the response (rather than just
returning) and avoid hanging requests when called from middleware like
dev-server.mjs.
In `@src/cjs-to-esm.ts`:
- Around line 128-140: The TryStatement visitor currently walks only node.block,
so require() calls in catch (node.handler) and finally (node.finalizer) are not
ignored; update the TryStatement handling in the walker to also walk
node.handler.body (if node.handler exists) and node.finalizer (if present) with
the same inner CallExpression visitor that pushes require() nodes into
ignoredExpressions (reuse the existing CallExpression check logic) so require()
calls inside catch and finally are excluded the same way as those in try.
- Around line 157-160: The code assumes node.arguments[0] exists when checking
CommonJS require() calls; update the guard around the specifier extraction so
you first verify node.arguments.length > 0 (and that specifier is defined)
before checking specifier.type === "StringLiteral" to avoid crashes on bare
require() calls—locate the block that defines const specifier =
node.arguments[0] and the subsequent if (specifier.type === "StringLiteral") so
you can replace it with a safe check that only pushes into callExpressions when
an argument exists and is a StringLiteral.
- Around line 109-116: The try/catch around parseEsm() swallows the explicit
mixed-module Error, so mixed ESM/CJS files are not rejected; update the catch in
the parse/validation block (the try that calls parseEsm(code) and throws new
Error("Cannot convert mixed modules")) to capture the error (e.g., catch (err))
and rethrow when it is the mixed-module rejection (check err.message === "Cannot
convert mixed modules" or instanceof a specific error), otherwise only suppress
or handle parseEsm-specific recoverable errors; ensure you adjust the catch
logic in that block so the thrown mixed-module error is not neutralized.
---
Nitpick comments:
In `@src/cjs-to-esm.ts`:
- Around line 167-177: The dedup map bug is caused by storing the spec under the
AST node object while reading by specifier.value, preventing duplicates from
being found; update the assignment to use the same string key as the lookup (use
specs.set(specifier.value, spec) instead of specs.set(specifier, spec)) so reads
via specs.get(specifier.value) and writes are consistent (leave id generation
and ns/count logic unchanged).
🪄 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: a17ce5b6-be36-43e4-a3fb-54d369e009ef
⛔ Files ignored due to path filters (7)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**src/__tests__/fixtures/isomorphic-commonjs-transpiled-default/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-commonjs-transpiled-default/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-commonjs-transpiled-default/node_modules/dep/index.jsis excluded by!**/node_modules/**and included by**src/__tests__/fixtures/isomorphic-commonjs-transpiled-default/node_modules/dep/package.jsonis excluded by!**/node_modules/**and included by**src/__tests__/fixtures/isomorphic-commonjs-transpiled-default/node_modules/transient-dep/index.jsis excluded by!**/node_modules/**and included by**src/__tests__/fixtures/isomorphic-commonjs-transpiled-default/node_modules/transient-dep/package.jsonis excluded by!**/node_modules/**and included by**
📒 Files selected for processing (11)
.changeset/spicy-zebras-buy.mdpackage.jsonsrc/__tests__/cjs-to-esm.test.tssrc/__tests__/fixtures/isomorphic-commonjs-transpiled-default/dev-server.mjssrc/__tests__/fixtures/isomorphic-commonjs-transpiled-default/server.mjssrc/__tests__/fixtures/isomorphic-commonjs-transpiled-default/src/index.jssrc/__tests__/fixtures/isomorphic-commonjs-transpiled-default/src/template.markosrc/__tests__/fixtures/isomorphic-commonjs-transpiled-default/test.config.tssrc/__tests__/main.test.tssrc/cjs-to-esm.tssrc/index.ts
Comment on lines
+3
to
+9
| export function handler(req, res) { | ||
| 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.
Non-root requests can hang because fallback is missing.
On Line 4, only / is handled. For other URLs, the function returns without ending the response or delegating. When called from middleware (e.g., dev-server.mjs), this can leave requests unresolved.
🔧 Proposed fix
-export function handler(req, res) {
+export function handler(req, res, next) {
if (req.url === "/") {
res.statusCode = 200;
res.setHeader("Content-Type", "text/html; charset=utf-8");
- template.render({}, res);
+ return template.render({}, res);
}
+ return next?.();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/fixtures/isomorphic-commonjs-transpiled-default/src/index.js`
around lines 3 - 9, The handler function currently only handles req.url === "/"
and returns for all other requests, leaving the response open; update the
handler (export function handler) to explicitly handle non-root requests by
setting an appropriate status (e.g., 404), writing a response body or calling
res.end(), or delegating to a next/middleware handler if available, so that for
any req.url other than "/" you always end the response (rather than just
returning) and avoid hanging requests when called from middleware like
dev-server.mjs.
Comment on lines
+128
to
+140
| TryStatement(node) { | ||
| walk(node.block, { | ||
| CallExpression(node) { | ||
| if ( | ||
| node.callee.type !== "Identifier" || | ||
| node.callee.name !== "require" | ||
| ) { | ||
| return; | ||
| } | ||
| ignoredExpressions.push(node); | ||
| }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
try/catch exclusion only covers the try block.
Current logic ignores require() in try { ... } only. Calls inside catch/finally are still transformed, which breaks the stated exclusion behavior.
Suggested fix
walk(ast, {
TryStatement(node) {
- walk(node.block, {
- CallExpression(node) {
- if (
- node.callee.type !== "Identifier" ||
- node.callee.name !== "require"
- ) {
- return;
- }
- ignoredExpressions.push(node);
- },
- });
+ const regions = [node.block, node.handler?.body, node.finalizer].filter(
+ Boolean,
+ ) as Node[];
+ for (const region of regions) {
+ walk(region, {
+ CallExpression(node) {
+ if (
+ node.callee.type !== "Identifier" ||
+ node.callee.name !== "require"
+ ) {
+ return;
+ }
+ ignoredExpressions.push(node);
+ },
+ });
+ }
},
});🤖 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 128 - 140, The TryStatement visitor currently
walks only node.block, so require() calls in catch (node.handler) and finally
(node.finalizer) are not ignored; update the TryStatement handling in the walker
to also walk node.handler.body (if node.handler exists) and node.finalizer (if
present) with the same inner CallExpression visitor that pushes require() nodes
into ignoredExpressions (reuse the existing CallExpression check logic) so
require() calls inside catch and finally are excluded the same way as those in
try.
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.
No description provided.