Skip to content

fix: improve commonjs translation in dev ssr#268

Merged
rturnq merged 1 commit intomainfrom
ssr-dev-cjs-fix
Apr 16, 2026
Merged

fix: improve commonjs translation in dev ssr#268
rturnq merged 1 commit intomainfrom
ssr-dev-cjs-fix

Conversation

@rturnq
Copy link
Copy Markdown
Contributor

@rturnq rturnq commented Apr 16, 2026

No description provided.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 16, 2026

🦋 Changeset detected

Latest commit: 4bca5d6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@marko/vite Patch

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

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 16, 2026

Walkthrough

The changeset introduces a patch update to the @marko/vite package's CommonJS-to-ES module translation helper. The __cjs_default__ interop helper in src/cjs-to-esm.ts was refactored to build a proxy-like module object using Object.create(null) instead of directly returning the namespace object. Properties are now re-exported via Object.defineProperty getters, with explicit handling of string exports (enumerable getters) and symbol exports (non-enumerable getters). An explicit __esModule flag is defined, and the control flow includes early return detection for existing __esModule markers.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive No pull request description was provided by the author, making it impossible to assess whether the description relates to the changeset. Add a pull request description that explains the motivation, approach, or impact of the CommonJS translation improvements in the dev SSR context.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: improve commonjs translation in dev ssr' accurately reflects the main change of updating the CommonJS-to-ES module helper for improved development SSR compatibility.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ssr-dev-cjs-fix

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/cjs-to-esm.ts`:
- Around line 30-34: The wrapper currently unconditionally sets enumerable: true
for each re-export (in the loop over Object.getOwnPropertyNames(ns)), which
changes non-enumerable source properties like __esModule; fix by reading the
original descriptor via Object.getOwnPropertyDescriptor(ns, k) and use its
enumerable (and configurable) value when calling Object.defineProperty on mod,
and preserve getter/value semantics by either copying the descriptor (but
replacing its get to forward into ns[k] if necessary) or by setting enumerable:
descriptor.enumerable and configurable: descriptor.configurable while keeping a
forwarding getter that returns ns[k]; reference symbols: mod, ns, k,
Object.getOwnPropertyNames, Object.getOwnPropertyDescriptor.
- Around line 22-27: The loop incorrectly skips properties with null/undefined
values due to the guard if (ns[k] == null) continue; which causes modules with
declared-but-uninitialized exports to be misclassified as default-only; remove
that null-check so the iteration always marks hasNamed = true for any property
from Object.getOwnPropertyNames(ns) (except the special keys "__esModule" and
"module.exports") and preserve the existing try/catch and the early return when
ns.__esModule is truthy; update the block around hasNamed, ns, k, and the
__esModule check accordingly.
🪄 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: 47371a17-6fcc-4c4b-a57e-e68981bf5d75

📥 Commits

Reviewing files that changed from the base of the PR and between f8ef9f4 and 4bca5d6.

📒 Files selected for processing (2)
  • .changeset/shaky-lilies-enter.md
  • src/cjs-to-esm.ts

Comment thread src/cjs-to-esm.ts
Comment thread src/cjs-to-esm.ts
@rturnq rturnq merged commit 963264a into main Apr 16, 2026
9 checks passed
@rturnq rturnq deleted the ssr-dev-cjs-fix branch April 16, 2026 21:59
@github-actions github-actions bot mentioned this pull request Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant