Skip to content

fix(trace): resolve traceInclude packages under minimal conditions (#57) - #58

Merged
pi0 merged 1 commit into
mainfrom
fix/nf3-trace-conditions
Jul 7, 2026
Merged

pi0 merged 1 commit into
mainfrom
fix/nf3-trace-conditions

Conversation

@pi0x

@pi0x pi0x commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #57.

traceInclude package resolution used the caller's raw conditions (e.g. Nitro's ["node"]). Packages whose exports map is keyed only by import/require — with no top-level default or ./package.json export (e.g. sharp) — match none of those keys, so resolveModulePath returns undefined and the <name>/package.json fallback also fails (such packages rarely expose package.json via exports). The package was then silently dropped from the trace.

The fix resolves traceInclude names with a permissive superset of the caller's conditions (import/require/default added). Since traceInclude only needs to locate a package in order to copy it wholesale, widening conditions here never changes downstream runtime behaviour.

const includeConditions = [
  ...new Set([...(opts.conditions || DEFAULT_CONDITIONS), "import", "require", "default"]),
];

Test plan

Added a regression test (test/trace.test.ts) with a new fixture @fixture/conditional-exports whose exports are keyed only by import/require. It is force-included with conditions: ["node"], and the test asserts both the package and its transitively-traced runtime dependency are copied. The test fails on main (package dropped) and passes with the fix.

Full suite: 41/41 passing; typecheck and lint clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved package tracing so force-included packages are resolved more reliably, even when their export conditions are limited.
    • Fixed an issue where some traced dependencies could be missed when using minimal resolution conditions.
  • Tests
    • Added regression coverage for force-included packages and their dependent runtime files.

…57)

`traceInclude` resolution used the caller's raw `conditions` (e.g. Nitro's
`["node"]`), which silently dropped packages whose `exports` are keyed only
by `import`/`require` — with no top-level `default` or `./package.json`
export (e.g. `sharp`). Resolve with a permissive superset of the caller's
conditions instead; since `traceInclude` only needs to locate a package to
copy it, widening conditions never changes runtime behaviour.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 7, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ab3738ab-11fe-4458-9050-8cda6cc57179

📥 Commits

Reviewing files that changed from the base of the PR and between 724511b and b067128.

⛔ Files ignored due to path filters (5)
  • test/fixture/node_modules/@fixture/conditional-dep/index.mjs is excluded by !**/node_modules/**
  • test/fixture/node_modules/@fixture/conditional-dep/package.json is excluded by !**/node_modules/**
  • test/fixture/node_modules/@fixture/conditional-exports/index.cjs is excluded by !**/node_modules/**
  • test/fixture/node_modules/@fixture/conditional-exports/index.mjs is excluded by !**/node_modules/**
  • test/fixture/node_modules/@fixture/conditional-exports/package.json is excluded by !**/node_modules/**
📒 Files selected for processing (3)
  • src/trace.ts
  • test/fixture/force-include-conditions.mjs
  • test/trace.test.ts

📝 Walkthrough

Walkthrough

Modifies traceNodeModules in src/trace.ts so traceInclude package resolution uses a widened, de-duplicated set of export conditions (caller conditions plus import, require, default, and DEFAULT_CONDITIONS) instead of raw caller conditions. Adds a fixture module and a regression test.

Changes

traceInclude Resolution Fix

Layer / File(s) Summary
Widen traceInclude resolution conditions
src/trace.ts
Builds includeConditions as a de-duplicated superset of caller conditions plus "import", "require", "default", and DEFAULT_CONDITIONS, and uses it in the resolveModulePath call for traceInclude resolution instead of raw opts.conditions.
Regression test and fixture
test/fixture/force-include-conditions.mjs, test/trace.test.ts
Adds a fixture module with a default export, and a test that verifies traceNodeModules resolves and copies a package with import/require-only exports when minimal conditions are supplied.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • unjs/nf3#50: Both modify traceNodeModules's handling of traceInclude resolution in src/trace.ts.
  • unjs/nf3#55: Both modify traceInclude handling in src/trace.ts, with the related PR extending transitive dependency tracing for force-included packages.
  • unjs/nf3#27: Both relate to traceInclude handling across the trace pipeline.

Suggested reviewers: pi0

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the traceInclude resolution fix under minimal conditions.
Linked Issues check ✅ Passed The code change and regression test directly address #57 by resolving traceInclude with broader conditions and verifying the fix.
Out of Scope Changes check ✅ Passed The changes are limited to the traceInclude fix and its test coverage, with no unrelated behavior changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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 fix/nf3-trace-conditions

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.

@codecov

codecov Bot commented Jul 7, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.13%. Comparing base (724511b) to head (b067128).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/trace.ts 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   79.54%   80.13%   +0.58%     
==========================================
  Files           5        5              
  Lines         577      579       +2     
  Branches      167      168       +1     
==========================================
+ Hits          459      464       +5     
+ Misses         85       84       -1     
+ Partials       33       31       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pi0
pi0 merged commit 130e28f into main Jul 7, 2026
8 of 9 checks passed
@pi0
pi0 deleted the fix/nf3-trace-conditions branch July 7, 2026 20:51
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.

traceInclude drops packages whose exports are keyed only by import/require (fail under minimal conditions)

2 participants