fix(resolve): prioritize main field over mainFields for requires

[dario-piotrowicz]

Description

When a require is performed the modules resolution currently prioritizes the mainFields option over the fact that a require has been used, this (especially given the default mainFields value) can lead to requires being resolved to browser entries instead of a potential cjs module entry. This PR addresses this by priotizing a potential main field over the mainField.

---

Details on how we encountered this issue

This was encountered as an issue when trying to run Remix code inside the workerd runtime via the environments API, there we encountered situations in which some modules imported via require would require packages but would receive esm entries instead of cjs ones, causing the following issue:

TypeError: Cannot use require() to import an ES Module.

This happened when
react-router-dom (package.json) was being required from @remix-run/react (code)

by adding some ad-hoc hacks to make the above work this issue would show up again
for react-router (packae.json) required from react-router-dom itself (code)

by also hacking the above we'd re-encounter the issue with:
@remix-run/router (package.json) being required from react-router (code)

and so on...

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 7484d8b: Open

suite	result	latest scheduled
ladle	❌ failure	✅ success
previewjs	✅ success	❌ failure
sveltekit	✅ success	❌ failure
vike	❌ failure	✅ success
vitest	❌ failure	❌ failure

✅ analogjs, astro, histoire, laravel, marko, nuxt, quasar, qwik, rakkas, remix, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress

[bluwy]

I'm not sure if this is correct. require() if building for browsers should use the browser entry if so. Bundlers can also handle if require resolves to an ESM file. Is there a specific package this is trying to fix? And could it use the exports field with the require condition instead? (if needed)

[dario-piotrowicz]

I'm not sure if this is correct. require() if building for browsers should use the browser entry if so. Bundlers can also handle if require resolves to an ESM file. Is there a specific package this is trying to fix? And could it use the exports field with the require condition instead? (if needed)

@bluwy I've just updated the PR's description with the package(s) that I was having issues with 👍

Also to give you more context, the whole thing here is that I'm trying to run a Remix application inside workerd via the Vite Environment API without prebundling the react code.

[bluwy]

I'm curious how this happens in the first place. According to

vite/packages/vite/src/node/ssr/fetchModule.ts

Lines 39 to 48 in c373251

	if (url[0] !== '.' && url[0] !== '/') {
	const { isProduction, root } = environment.config
	const { externalConditions, dedupe, preserveSymlinks } =
	environment.options.resolve
	const resolved = tryNodeResolve(
	url,
	importer,
	{
	mainFields: ['main'],

Only main field should be used when resolving bare imports. If it's not a bare import (e.g. it's in noExternal which Vite rewrites to a different path. Or a plugin resolves it to something else. Or perhaps you're testing the packages linked locally?), then Vite will process these packages, however Vite won't handle CJS and require() correctly.

In that case, I'd recommend passing the packages to resolve.external in the environment api. But I guess with the workerd environment, it's not possible to do so? Then prebundling seems like the only solution here 🤔

This happened when
react-router-dom (package.json) was being required from @remix-run/react (code)

If the problem started from this, would it be possible for remix-run/react to import react-router-dom first?