Skip to content

feat: Ensure module-sync conditions also trace cjs fallback#550

Merged
jeffsee55 merged 16 commits intomainfrom
fix-module-sync-for-cjs
Nov 24, 2025
Merged

feat: Ensure module-sync conditions also trace cjs fallback#550
jeffsee55 merged 16 commits intomainfrom
fix-module-sync-for-cjs

Conversation

@jeffsee55
Copy link
Contributor

@jeffsee55 jeffsee55 commented Nov 18, 2025

fix: Include fallback condition when evaluating module-sync exports

Previously, when resolving the module-sync condition, we only traced the module-sync file itself, not the fallback (require or default condition). This caused runtimes with require(esm) disabled to be unable to find the module.

Also, fixed a bug in the module-sync-condition-cjs test where:

  • The input was importing from test-pkg-sync-es instead of test-pkg-sync-cjs (wrong package name)
  • The test was running on both Node 20 and Node 22, producing inconsistent results
  • Changed input to use require() syntax (appropriate for CJS packages)
  • Now only runs on Node 22+ where module-sync is supported
  • Created separate module-sync-condition-cjs-node20 test for Node 20 behavior

Test Coverage

Test Node Syntax Files Traced Reason
module-sync-condition-cjs 22+ require() module-sync.js, fallback.js Uses module-sync + fallback for require(esm) opt-out
module-sync-condition-cjs-node20 20 require() fallback.js Falls back to default (no module-sync support)
module-sync-condition-es 22+ import module-sync.js, fallback.js Uses module-sync + fallback for require(esm) opt-out
module-sync-condition-es-node20 20 import import.js Falls back to import condition (no module-sync support)

Also adjusts jest thresholds, here are the main paths this PR added that it did not add test coverage for, mostly error handling

Screenshot 2025-11-20 at 11 57 37 AM

…odule-sync condition, causing runtimes with the require(esm) flag disabled to be unable to find a module
@jeffsee55 jeffsee55 changed the title Fix issue were we didn't include a CJS fallback when evaulating the m… fix: Ensure module-sync conditions also trace cjs fallback Nov 19, 2025
@jeffsee55 jeffsee55 marked this pull request as ready for review November 19, 2025 20:59
@jeffsee55 jeffsee55 requested review from a team, icyJoseph, ijjk and styfle as code owners November 19, 2025 20:59
@styfle styfle changed the title fix: Ensure module-sync conditions also trace cjs fallback feat: Ensure module-sync conditions also trace cjs fallback Nov 20, 2025
@jeffsee55 jeffsee55 merged commit 684032b into main Nov 24, 2025
12 checks passed
@jeffsee55 jeffsee55 deleted the fix-module-sync-for-cjs branch November 24, 2025 18:04
@github-actions
Copy link

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jeffsee55 added a commit that referenced this pull request Nov 26, 2025
…allback (#557)

The code (added [here](#550)) was
previously only adding the fallback when the condition was a string, but
wasn't handling `{`"default": {"default": "./fallback.js"}}`, as is used
in [this
package](https://www.npmjs.com/package/@mjackson/node-fetch-server/v/0.2.0?activeTab=code)
serhalp added a commit to netlify/build that referenced this pull request Dec 4, 2025
We had [previously pinned to 0.29.4](#6610) because [0.30 introduced a bug](https://answers.netlify.com/t/react-router-app-crashing-because-it-cant-find-package/155391).

This was fixed in vercel/nft#550, released in 1.1.0.

I've confirmed this:

1. Clone https://github.com/netlify/react-router-template/
2. `npm i`
4. ✅ `rm -rf .netlify/ && npx -y netlify@latest deploy --skip-functions-cache`
5. ✅ `rm -rf .netlify/ && npx -y netlify@23.0.0 deploy --skip-functions-cache`
6. ✅ `rm -rf .netlify/ && npx -y netlify@23.1.3 deploy --skip-functions-cache`
7. ❌ `rm -rf .netlify/ && npx -y netlify@23.1.2 deploy --skip-functions-cache`
8. Clone this branch, `npm i && npm run build`. Clone netlify-cli, `npm link ../build/packages/zip-it-and-ship-it`.
9. ✅ `rm -rf .netlify/ && ../cli/bin/run.js deploy --skip-functions-cache`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants