fix(resolve): use only root package.json as exports source

[Andarist]

Description

I'm Emotion's maintainer and got this bug report.

I've narrowed this down to our @emotion/styled/base not being resolved correctly by Vite. For compatibility reasons we have @emotion/styled/base/package.json in our package (can be viewed here) - from which Vite tries to read package.json#exports. However, this is not how the node resolution algorithm works. exports are only read from the root package.json and we have appropriate entries there (see here).

The pseudo algorithm for this resolution can be found in the node docs here. We should focus on the LOAD_PACKAGE_EXPORTS procedure here - note how its 3rd step is this:

Parse DIR/NAME/package.json, and look for "exports" field.

This doesn't loop anyhow, nor it doesn't include SUBPATH here. This means that exports are only read from the root package.json

This can be verified against the node's runtime or in their source code here. Note how, in here, /base becomes the expansion and that isn't used anyhow by pkgPath, pkg and packageExportsResolve

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[bluwy]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
astro	❌ failure
histoire	✅ success
iles	✅ success
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt-framework	✅ success
previewjs	✅ success
qwik	✅ success
rakkas	✅ success
sveltekit	✅ success
vite-plugin-ssr	❌ failure
vite-plugin-react	✅ success
vite-plugin-react-pages	✅ success
vite-plugin-react-swc	✅ success
vite-plugin-svelte	✅ success
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success
windicss	✅ success

[bluwy]

Looks like this broke preact imports. I haven't looked too deep yet, but I may have missed something in the resolving code that's causing this. A unique thing with Preact's package is that all nested package.json have exports too. If I find some time I can look into this next week.

[Andarist]

I'm investigating this - gonna have to get back to it later but so far I've noticed one difference. Without this patch, I get such values in this function:

{
  pkgId: 'preact',
  rootPkg: null,
  id: 'preact/jsx-dev-runtime',
  importer: undefined
}

whereas with this patch rootPkg is being resolved.

[Andarist]

I got almost to the bottom of the problem - and I'm afraid that I'm not sure how to solve this because Vite's resolver is fundamentally incompatible with how package.json#exports work in other tools (where I consider the node to define this "spec" and you deviate from it).

With the old code, you don't consider import 'preact/hooks' to be a "deep import" so you don't end up using resolveDeepImport:

vite/packages/vite/src/node/plugins/resolve.ts

Lines 716 to 718 in f12a1ab

	const isDeepImport = unresolvedId !== nestedPath
	if (isDeepImport) {
	resolveId = resolveDeepImport

This happens because rootPkgId is set to a wrong value here:

vite/packages/vite/src/node/plugins/resolve.ts

Line 681 in f12a1ab

pkgId = rootPkgId

since the "root" has to be determined as preact/hooks instead of preact.

So you try to resolve this with resolvePackageEntry, you end up resolving exports from the incorrect package.json file here and you end up with .mjs file (correctly):

vite/packages/vite/src/node/plugins/resolve.ts

Lines 928 to 932 in f12a1ab

	// resolve exports field with highest priority
	// using https://github.com/lukeed/resolve.exports
	if (data.exports) {
	entryPoint = resolveExports(data, '.', options, targetWeb)
	}

But you end up overriding, what should be the "final" return value, with the package.json#module value here:

vite/packages/vite/src/node/plugins/resolve.ts

Lines 983 to 991 in f12a1ab

	if (!entryPoint || entryPoint.endsWith('.mjs')) {
	for (const field of options.mainFields) {
	if (field === 'browser') continue // already checked above
	if (typeof data[field] === 'string') {
	entryPoint = data[field]
	break
	}
	}
	}

So you actually end up resolving the commonjs file for preact/hooks here.

With my patch though... we end up using resolveDeepImport and you just stick to the decision based on the package.json#exports (from the correct package.json):

vite/packages/vite/src/node/plugins/resolve.ts

Line 1116 in f12a1ab

const exportsId = resolveExports(data, file, options, targetWeb)

Both pacakge.json files there are configured in a way that they both point to the same files so you end up resolving to the very same .mjs file. The difference is that one of those resolvers has a custom overriding logic (not spec-compliant) and the other one doesn't.

[Andarist]

Probably a fix could look like this:

• introduce the same custom .mjs overriding logic into resolveDeepImport
• pass around both rootPkg and nearestPkg
• if we resolve .mjs from the rootPkg.data.exports - try to override them based on nearestPkg.data.module (and other mainFields). This would be required since rootPkg.data.module can only point to "its own" file (for the root entry) but it can't reference anyhow a nested one, that can only be configured by nearestPkg (for its own "deep entry")

I don't understand why you need this overriding logic in the first place though. From my PoV... it just shouldn't be there. package.json#exports shouldn't be overridden. This deviates from every other tool in the ecosystem, it might create resolution bugs. I would consider the fact that a package can load "successfully" in Vite while failing to load in other tools a bad thing. It just fractures the ecosystem even further and might lead people to believe that resolution works in a different way than it actually does.

[bluwy]

Thanks for digging into this! I agree that the .mjs override is incorrect, we have #11114 tracking this which was supposed to be fixed in Vite 4, but was reverted.

If we re-apply #11234 (the fix), I believe that should fix the preact issue? If so, I think we could land both of these fixes and try to figure out the Vue issue in #11114, in the next Vite minor. The current behaviour deviates from Node resolution algorithm so we can consider this a bug fix.

[bluwy]

Here's the run with this PR + #11234: https://github.com/vitejs/vite-ecosystem-ci/actions/runs/3822217463. We got the same fail from Histoire (as few weeks ago) which I believe stems from vite-node. I'm not completely sure where can be fixed yet.

[Andarist]

If we re-apply #11234 (the fix), I believe that should fix the preact issue?

Not necessarily - but that's super hard to tell without actually trying it. The issue with Preact is that you end up loading both CJS and ESM of preact/hooks. This introduces a dual package problem.

IIRC the CJS file is loaded by preact-render-to-string. This is treated as an external package in node (correctly) and thus you don't control how this is resolved because it's resolved by node. But this one is loaded from here and using a dynamic import - this in turn means that it should be loaded using the import condition (regardless of the module format of the importer). This is described here:

"import" - matches when the package is loaded via import or import(), or via any top-level import or resolve operation by the ECMAScript module loader. Applies regardless of the module format of the target file. Always mutually exclusive with "require".

Somehow you manage to load the CJS file for preact/hooks without my patch. The argument given to this dynamicImport is already an absolute path of this file - and with my patch it's also an absolute path, just a different one (the one contained in the import condition for the preact/hooks entrypoint).

[bluwy]

Thanks for the explanation! Yeah that makes sense as to why it's failing now. I've also tested it in the comment above yours (I sent that 1 minute before yours 😬), and it seems to fix it. I think we should merge this in 4.1, which the beta should come soon this week or the next. I'll loop in Patak when he gets back and we can coordinate this further.

[patak-dev]

Hey, I'm back 👋🏼
And I agree, let's try to get this one in 4.1 👍🏼