Skip to content

Conversation

@medz
Copy link
Contributor

@medz medz commented Feb 10, 2026

πŸ”— Linked issue

#4010

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This fixes the rolldown regression reported in #4010 for Cloudflare worker targets.

  • Add a rolldown pre-resolver for require-call to resolve package exports with require-first conditions (instead of accidentally selecting import entries for CJS requires).
  • Enable rolldown esmExternalRequirePlugin for nodeless builds so Node builtins used via CJS require() are emitted as ESM-compatible externals for worker runtimes.
  • Keep Node-target behavior unchanged.
  • Add unit coverage for dual export (import/require) resolution behavior.

Validated with the minimal repro:
medz/nitro-issue-4010-pg-cloudflare-repro

NITRO_BUILDER=rolldown nitro build + wrangler dev now starts successfully and serves requests (no require("events") startup failure).

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@medz medz requested a review from pi0 as a code owner February 10, 2026 10:01
@vercel
Copy link

vercel bot commented Feb 10, 2026

@medz is attempting to deploy a commit to the Nitro Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link

coderabbitai bot commented Feb 10, 2026

πŸ“ Walkthrough

Walkthrough

This PR introduces require-condition resolution for Rolldown in Nitro's build system. It adds a new plugin module that conditionally resolves CommonJS require calls based on export conditions, integrates it into the Rolldown configuration, and provides comprehensive unit tests for the new functionality.

Changes

Cohort / File(s) Summary
Config Rewiring
src/build/rolldown/config.ts
Introduced dynamic plugin and externals composition: added requireConditionResolver plugin, conditionally included esmExternalRequirePlugin, precomputed builtinExternals for both standard and node:-prefixed variants, and consolidated plugins array initialization.
Require-Condition Plugin
src/build/rolldown/require-condition.ts
New plugin module for conditional require() path resolution. Exports getRequireConditionNames() to compute effective condition names, resolveRequireCallPath() to resolve CommonJS imports with conditions, and requireConditionResolver() factory to integrate with Nitro's build context.
Plugin Tests
test/unit/rolldown-require-condition.test.ts
Unit tests for require-condition resolution, including dual-entry package resolution preference tests and validation of builtin/relative import skipping behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

πŸš₯ Pre-merge checks | βœ… 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
βœ… Passed checks (2 passed)
Check name Status Explanation
Title check βœ… Passed The title follows conventional commits format with 'fix' prefix and clearly describes the main change: fixing CJS require handling for nodeless worker builds.
Description check βœ… Passed The description is directly related to the changeset, providing context about the bug fix, implementation details, and validation.

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

✨ Finishing touches
  • πŸ“ Generate docstrings
πŸ§ͺ Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❀️ Share

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

Copy link

@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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/build/rolldown/config.ts (1)

52-57: ⚠️ Potential issue | 🟑 Minor

Remove debug console.log β€” likely a leftover.

Line 54 unconditionally logs warning.code to stdout on every rolldown warning. This looks like a debug artifact. The warn(warning) call on the next line already handles emitting the warning properly. Per coding guidelines, build code should use consola or nitro.logger for logging.

Proposed fix
     onwarn(warning, warn) {
       if (!base.ignoreWarningCodes.has(warning.code || "")) {
-        console.log(warning.code);
         warn(warning);
       }
     },

As per coding guidelines, src/**/*.{js,ts}: "Use consola for logging in build/dev code; use nitro.logger when available".

@pkg-pr-new
Copy link

pkg-pr-new bot commented Feb 10, 2026

Open in StackBlitz

npm i https://pkg.pr.new/nitrojs/nitro@4019

commit: ff04bbb

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