fix: unoptimized package can resolve to optimized deps

[csr632]

Description

https://github.com/vitejs/vite/pull/11290 make tryOptimizedResolve stricter, removing some wrong code path that would resolve to optimized deps without considering the importer.

But the wrong code path hid another bug in tryOptimizedResolve. There are some cases that relied on the wrong code path to behave normally. Removing the code path in 4.0.1 make these cases fail, which reveal the new bug.

The new bug is: resolveFrom always use package.json#main as the main field, which is not aligned with the vite's built in resolve algorithm (which is used to compute optimizedData.src). So tryOptimizedResolve doesn't match any package with package.json#module field: https://github.com/vitejs/vite/blob/0a07686f651b8583e8091ada146b96576e8501e9/packages/vite/src/node/plugins/resolve.ts#L911

Here is an example(the test case in this PR):

project
  - package-a (optimized. `package.json#module` as main field)
  - package-b (not optimized. peer-depend on package-a.)

Without this fix, the project import package-a and gets the optimized bundle. But package-b get the unoptimized module. So it ends up with two instance of package-a.

Fix: https://github.com/solidjs/solid/issues/1426

https://github.com/vitejs/vite/pull/11290 was reverted to fix(hide) the bug in https://github.com/solidjs/solid/issues/1426 . This PR cherry-pick https://github.com/vitejs/vite/pull/11290 and add more fix around it (fix https://github.com/solidjs/solid/issues/1426).

Additional context

---

What is the purpose of this pull request?

• [x] Bug fix
• [ ] New Feature
• [ ] Documentation update
• [ ] Other

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

• [x] Read the Contributing Guidelines.
• [x] Read the Pull Request Guidelines and follow the PR Title Convention.
• [x] 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).
• [x] Ideally, include relevant tests that fail without this PR but pass with it.

[csr632]

Ideally we should drop resolveFrom entirely and always use tryNodeResolve. But I feel tryNodeResolve very complicated and subtle to use... Callers have to collect arguments carefully and know every edge cases of it. 🤔

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci[bot]]

📝 Ran ecosystem CI: Open

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

[patak-dev]

Thanks for working on this regerssion! I think that if you confirmed the linked PR was causing the issue in solid we should revert it while you check this. It seems that this PR would cause regressions in other CIs. What do you think?

[csr632]

Thanks for working on this regerssion! I think that if you confirmed the linked PR was causing the issue in solid we should revert it while you check this. It seems that this PR would cause regressions in other CIs. What do you think?

I agree.

[csr632]

@bluwy Could you take a look at this PR?

Also need some help on the failing CI of vite-plugin-svelte above. The histoire CI error also seems to related to svelte.

[csr632]

I have added back the test and the fix reverted by https://github.com/vitejs/vite/pull/11412

Also tested with the solid template npm init solid@latest. The solid bug is caused by the same reason as the new test case I described above. And it will be fixed by this PR.

[csr632]

/ecosystem-ci run

[patak-dev]

We really need to change that emoji reaction 😅

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci[bot]]

📝 Ran ecosystem CI: Open

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

[csr632]

Ideally we should drop resolveFrom entirely and always use tryNodeResolve. But I feel tryNodeResolve very complicated and subtle to use... Callers have to collect arguments carefully and know every edge cases of it. 🤔

I just push a commit that split tryNodeResolve into two parts:

• tryNodeResolveCore: remove logic related to depsOptimizer, ssr and external. These logic happened to located at the bottom of previous tryNodeResolve, so it is very natural to move them away.
• tryNodeResolve: do exactly what the previous tryNodeResolve did. Call tryNodeResolveCore internally.

Now tryNodeResolveCore is easier to reuse. It has simpler goal and much less arguments! Here are the places that we can use tryNodeResolveCore instead of tryNodeResolve:

• https://github.com/csr632/vite/blob/e58a4f00e201e9c0d43ddda51ccac7b612d58650/packages/vite/src/node/config.ts#L1000
• https://github.com/csr632/vite/blob/e58a4f00e201e9c0d43ddda51ccac7b612d58650/packages/vite/src/node/ssr/ssrModuleLoader.ts#L232
• https://github.com/csr632/vite/blob/b51a3129b798345c1326a041db3c02014ff377a4/packages/vite/src/node/plugins/resolve.ts#L963

@patak-dev @bluwy What do you think about this? If you agree on this, I can further polish tryNodeResolveCore. (I tried to keep the git diff simple in the above commit).

[bluwy]

Thanks for keeping the git diff clean for this one. I've been thinking of a cheaper way around this too, and maybe this works:

1. Keep using resolveFrom, but append package.json when resolving.
2. Retrieve the path and strip of package.json.
3. Use that path and compare via optimizedData.src.startsWith(resolvedSrc) instead of ===

This should work for most case unless there's a bizarre dependency loop that messes the startsWith, but that feels rare enough that we can do a proper fix later.

---

If that idea doesn't work, I think the split of tryNodeResolve is great too, and might be the only other way to fix this.

FWIW currently the esbuild scanner uses

https://github.com/vitejs/vite/blob/80f405ec22cb589d6ba5a52f7b7d8e8ed71ad1ff/packages/vite/src/node/optimizer/scan.ts#L173-L193

but we can't do that as it likely has perf downsides and overkill in most cases. so tryNodeResolve should be enough.

[csr632]

Use that path and compare via optimizedData.src.startsWith(resolvedSrc) instead of ===

I think it will have issue when package has multiple entries. For example, optimizedData.src is /path/to/test-package/entry1 but this tryOptimizedResolve(depsOptimizer, id, importer) is a request for test-package/entry2. In this case, they shouldn't match. But this method will compute resolvedSrc to /path/to/test-package and evaluate optimizedData.src.startsWith(resolvedSrc) to true.

[bluwy]

Ah you're right 🥲 It looks like we need to always resolve the package then, and using tryNodeResolve to ensure it's the same. Let's go with that then.

[csr632]

@bluwy Hi! This PR is now ready to be reviewed.

[csr632]

@patak-dev @bluwy Hi! Could we try merging this fix in 4.1.0?

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci[bot]]

📝 Ran ecosystem CI: Open

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

[patak-dev]

@csr632 thanks for your work here, this path looks good to me. We discussed with the team about moving forward, but the change is a bit bigger than I thought initially. I would also like to give @bluwy an opportunity to do a proper review here. We plan to release beta.1 for 4.1 so I think we should push this PR for 4.2.

[bluwy]

Sorry I had only had the time to do a full review. The refactor to make this work looks great to me, but it seems like this fails for vitepress and vite-plugin-svelte according to the ecosystem CI. I'm not sure where yet this is causing issues.

[patak-dev]

I'll remove the 4.2 milestone, as the issues with vite-plugin-svelte and vitepress won't let us move forward with this in a minor. I'll add it to the Vite 5 milestone where we could merge and work with them to solve the issue, but we can still aim for 4.3 if these issues are resolved.

[bluwy]

Putting a note that I'll probably come around and rebase this soon. I'm refactoring some resolve stuff, and if all goes well this PR would help remove our reliance on the resolve dep.

[bluwy]

Ah my wording in the other PR accidentally closed this. Nonetheless, I'm still working on a fix locally and it'll be quite different from this PR, so there will be a new one soon.