Skip to content

fix(core): scope source conditions to source modules - #36725

Open
llwt wants to merge 5 commits into
masterfrom
fix/nxc-4816-scoped-source-resolver
Open

fix(core): scope source conditions to source modules#36725
llwt wants to merge 5 commits into
masterfrom
fix/nxc-4816-scoped-source-resolver

Conversation

@llwt

@llwt llwt commented Aug 20, 2026

Copy link
Copy Markdown
Member

Current Behavior

When a local Nx plugin is loaded from TypeScript source, its transitive workspace-package imports do not inherit the workspace's customConditions. In the daemon, Node can therefore select the package's default built export instead of its source export, causing sync generators to fail with ERR_MODULE_NOT_FOUND when that built output is absent.

Starting the entire daemon with the workspace conditions fixes the failing import, but also changes resolution globally for built generators, executors, and unrelated daemon code.

Expected Behavior

Apply workspace export conditions only to known workspace-package imports reached from an explicitly source-loaded plugin, generator, or executor entry.

This change:

  • tracks the source module graph, including lazy relative imports;
  • preserves the existing conditions on isolated plugin workers;
  • leaves built consumers on Node's default conditions;
  • refreshes conditions from the workspace tsconfig and cleans plugin resolver state on unload;
  • keeps condition-aware resolution independent from TypeScript transpilation.

The reproduction using published Nx 23.1.1 fails with the daemon enabled. After copying this branch's locally built nx package into the reproduction, nx sync succeeds and runs the sync generator.

Validation

  • pnpm nx test nx --runTestsByPath src/plugins/js/utils/register.spec.ts src/project-graph/plugins/resolve-plugin.spec.ts — 43 tests passed
  • pnpm nx run-many -t build,lint -p nx — passed
  • pnpm nx run e2e-plugin:typecheck — passed
  • pnpm nx prepush — passed
  • pnpm nx affected -t build,test,lint — 121/123 tasks passed; the remaining nx:test and devkit:test failures were unrelated local ANSI/hash/timing snapshot differences
  • pnpm nx affected -t e2e-local — blocked before e2e execution because the local registry could not resolve the current @nx version

Related Issue(s)

Fixes NXC-4816.

Alternative to #36708, following the source-scoped resolver proposal from the Linear discussion.

@netlify

netlify Bot commented Aug 20, 2026

Copy link
Copy Markdown

Deploy Preview for nx-docs ready!

Name Link
🔨 Latest commit 5337f3a
🔍 Latest deploy log https://app.netlify.com/projects/nx-docs/deploys/6a8716653ff42800089117e7
😎 Deploy Preview https://deploy-preview-36725--nx-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify

netlify Bot commented Aug 20, 2026

Copy link
Copy Markdown

Deploy Preview for nx-dev ready!

Name Link
🔨 Latest commit 5337f3a
🔍 Latest deploy log https://app.netlify.com/projects/nx-dev/deploys/6a871665470359000873ff6b
😎 Deploy Preview https://deploy-preview-36725--nx-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@nx-cloud

nx-cloud Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

View your CI Pipeline Execution ↗ for commit 5337f3a

Command Status Duration Result
nx affected --targets=lint,oxlint,test,build,e2... ✅ Succeeded 48m View ↗
nx run-many -t check-imports check-lock-files c... ✅ Succeeded 5s View ↗
nx-cloud record -- pnpm nx-cloud conformance:check ✅ Succeeded 43s View ↗
nx build workspace-plugin ✅ Succeeded <1s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded 17s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 5s View ↗

☁️ Nx Cloud last updated this comment at 2026-08-20 15:52:39 UTC

@llwt
llwt marked this pull request as ready for review August 20, 2026 12:35
@llwt
llwt requested a review from a team as a code owner August 20, 2026 12:35
@llwt
llwt requested a review from leosvelperez August 20, 2026 12:35
@llwt llwt changed the title Fix/nxc 4816 scoped source resolver core: scope source conditions to source modules Aug 20, 2026

@nx-cloud nx-cloud Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nx Cloud has identified a flaky task in your failed CI:

🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.

Nx Cloud View detailed reasoning in Nx Cloud ↗


🎓 Learn more about Self-Healing CI on nx.dev

nx-cloud Bot and others added 2 commits August 20, 2026 13:31
…h resolver

Node < 22.19 / < 24.5 hands CJS resolve hooks a Set and forwards the hook's
return value to resolvers that call `.has()`, so returning an array threw
`TypeError: conditions.has is not a function` for every workspace-package
`require()` from a source-loaded entry on those runtimes, including when the
process was already started with `--conditions`. Return the shape we were
given, and state the real runtime floor in the doc comment: ESM imports only
pick up hook-supplied conditions from 22.19 / 24.5.
@llwt llwt changed the title core: scope source conditions to source modules fix(core): scope source conditions to source modules Aug 20, 2026
@llwt

llwt commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

Adversarial review (diff + ticket repro + the shipped hook run on real Node 22.18 / 24.4 / 26.3). Verdict before the pushed fix: needs-changes; after it: no blocking findings from my side.

Fixed in 5337f3a (pushed here)

  • appendConditions returned an Array. Node < 22.19 / < 24.5 hands CJS resolve hooks a SafeSet and forwards the hook's return to resolvers that call .has(), so every workspace-package require() from a source-loaded entry threw TypeError: conditions.has is not a function on those runtimes, including when --conditions/NODE_OPTIONS were already set (base skipped the hook in that case; this PR dropped the skip). It now returns the shape it was given; unit test added with Set-shaped conditions.
  • Doc comment claimed a 22.15+ floor. Corrected: CJS works from 22.15, but ESM imports only pick up hook-supplied conditions from 22.19 / 24.5 (Node's ESM loader drops them before that; no 23.x has the fix), so in-process ESM keeps default conditions below that. Isolated workers still get --conditions at spawn.

Verified: base --conditions daemon spawn reproduces the ticket's exact error and the HEAD spawn clears it (Node 26.3 and 24.4); register.spec.ts 39/39; CI at a8b19b6 ran the reworked plugin e2e green (the red watch and create-github-release e2e tasks are untouched by this diff).

Open for maintainers

  1. Built generators/executors now resolve workspace imports to dist even with a source condition: the repro's "build both packages" step stays mandatory, and a fresh clone with unbuilt dist fails the sync generator. If that is the intended contract, the PR body should say so.
  2. Node floor: the repo declares no engines, and the in-process ESM path is only correct from 22.19 / 24.5. Gate, or document.
  3. Non-blocking: schema-utils.ts registers the resolver with no cleanup (deliberate leak) and re-walks getWorkspacePackagesMetadata per factory call instead of the memo at line 251; registerHooks lost base's try/catch; the e2e exercises the isolated-worker half only (CI is Node 26.3), so a source-factory sync-generator case would cover the new hook end to end.

Retitled with fix(core): for the "Validate PR Title" check.

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