fix: unoptimized package can resolve to optimized deps by csr632 · Pull Request #11410 · vitejs/vite

csr632 · 2022-12-18T07:13:19Z

Description

#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:

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

Line 911 in 0a07686

if (optimizedData.src === resolvedSrc) {

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: solidjs/solid#1426

#11290 was reverted to fix(hide) the bug in solidjs/solid#1426 . This PR cherry-pick #11290 and add more fix around it (fix solidjs/solid#1426).

Additional context

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 PR Title 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.

csr632 · 2022-12-18T07:17:59Z

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

    if (resolvedSrc == null) {
      try {
-        // this may throw errors if unable to resolve, e.g. aliased id
-        resolvedSrc = normalizePath(resolveFrom(id, path.dirname(importer)))


The bug come from this line: resolveFrom always use package.json#main as main field, not respecting vite's main fields (module field and custom mainFields). So the resolvedSrc here is not aligned with optimizedData.src.

This PR fix it by changing this line to use tryNodeResolve which is aligned with the resolve result during deps optimization.

csr632 · 2022-12-18T07:27:10Z

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-cat · 2022-12-18T07:27:35Z

/ecosystem-ci run

vite-ecosystem-ci · 2022-12-18T07:27:57Z

📝 Ran ecosystem CI: Open

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

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

patak-cat · 2022-12-18T07:48:30Z

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 · 2022-12-18T08:13:10Z

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 · 2022-12-18T08:24:27Z

@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 · 2022-12-18T14:25:32Z

I have added back the test and the fix reverted by #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 · 2022-12-18T14:26:37Z

/ecosystem-ci run

patak-cat · 2022-12-18T14:36:16Z

We really need to change that emoji reaction 😅

patak-cat · 2022-12-18T14:36:24Z

/ecosystem-ci run

vite-ecosystem-ci · 2022-12-18T14:36:51Z

📝 Ran ecosystem CI: Open

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

csr632 · 2022-12-18T15:39:37Z

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:

@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 · 2022-12-20T15:00:06Z

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:

Keep using resolveFrom, but append package.json when resolving.
Retrieve the path and strip of package.json.
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

vite/packages/vite/src/node/optimizer/scan.ts

Lines 173 to 193 in 80f405e

    
           const resolve = async ( 
        
             id: string, 
        
             importer?: string, 
        
             options?: ResolveIdOptions, 
        
           ) => { 
        
             const key = id + (importer && path.dirname(importer)) 
        
             if (seen.has(key)) { 
        
               return seen.get(key) 
        
             } 
        
             const resolved = await container.resolveId( 
        
               id, 
        
               importer && normalizePath(importer), 
        
               { 
        
                 ...options, 
        
                 scan: true, 
        
               }, 
        
             ) 
        
             const res = resolved?.id 
        
             seen.set(key, res) 
        
             return res 
        
           }

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

csr632 · 2022-12-25T12:11:25Z

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 · 2022-12-27T17:03:20Z

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 · 2023-01-08T15:09:09Z

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

  if (id.startsWith('node:') || isBuiltin(id)) {
    url = id
  } else {
-    const resolved = tryNodeResolve(


Calling tryNodeResolve with 4 args has same effect as calling tryNodeResolveCore, but tryNodeResolve has more mental burden. So I turn it into tryNodeResolveCore call here.

csr632 · 2023-01-08T15:10:13Z

packages/vite/src/node/config.ts

              }

              const isIdESM = isESM || kind === 'dynamic-import'
-              let idFsPath = tryNodeResolve(


Calling tryNodeResolve with 4 args has same effect as calling tryNodeResolveCore, but tryNodeResolve has more mental burden. So I turn it into tryNodeResolveCore call here.

csr632 · 2023-01-08T15:21:59Z

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

csr632 · 2023-01-26T07:10:32Z

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

patak-cat · 2023-01-26T09:52:45Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-01-26T09:53:13Z

📝 Ran ecosystem CI: Open

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

patak-cat · 2023-01-26T10:05:57Z

@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 · 2023-01-26T11:57:04Z

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-cat · 2023-03-06T16:23:06Z

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 · 2023-03-16T12:49:41Z

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 · 2023-04-07T02:25:09Z

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.

csr632 commented Dec 18, 2022

View reviewed changes

packages/vite/src/node/plugins/resolve.ts Outdated Show resolved Hide resolved

csr632 mentioned this pull request Dec 18, 2022

Vite 4.0.1 and "You appear to have multiple instances of Solid" solidjs/solid#1426

Closed

patak-cat mentioned this pull request Dec 18, 2022

fix: revert #11290 #11412

Merged

4 tasks

csr632 force-pushed the feat/fix-resolve-optimized branch from fff689f to 68bdeef Compare December 18, 2022 13:46

csr632 force-pushed the feat/fix-resolve-optimized branch from 90a9767 to b51a312 Compare December 18, 2022 15:52

csr632 added 5 commits January 6, 2023 00:38

fix: wrongly resolve to optimized doppelganger (vitejs#11290)

82ad4bf

test: add test that fail to resolve to optimized dep

d5a2d78

fix: unoptimized package can resolve to optimized deps

7115fbf

update pnpm-lock

83e328e

split tryNodeResolve into tryNodeResolveCore

9e77ae1

csr632 force-pushed the feat/fix-resolve-optimized branch from b51a312 to 9e77ae1 Compare January 5, 2023 16:41

csr632 added 3 commits January 8, 2023 22:28

tryOptimizedResolve use tryNodeResolveCore

7944bcb

improve tryNodeResolveCore return value

c58bb0c

call tryNodeResolveCore insead of tryNodeResolve to reduce mental burden

3b57c27

csr632 commented Jan 8, 2023

View reviewed changes

cleanup

a1d50f6

csr632 mentioned this pull request Jan 26, 2023

fix: wrongly resolve to optimized doppelganger #11290

Merged

9 tasks

chore: merge main

62f9cb2

patak-cat added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jan 26, 2023

patak-cat added this to the 4.2 milestone Jan 26, 2023

patak-cat modified the milestones: 4.2, 5.0 Mar 6, 2023

bluwy self-assigned this Mar 16, 2023

bluwy mentioned this pull request Mar 16, 2023

perf(resolve): improve package.json resolve speed #12441

Merged

9 tasks

This was referenced Mar 30, 2023

Monorepo project dedupes dependencies even when not specified #9948

Open

refactor: use simpler resolve for nested optimized deps #12770

Merged

patak-cat closed this in #12770 Apr 6, 2023

bluwy removed this from the 5.0 milestone Jul 31, 2023

Uh oh!

Conversation

csr632 commented Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

What is the purpose of this pull request?

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

Uh oh!

csr632 Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csr632 commented Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patak-cat commented Dec 18, 2022

Uh oh!

vite-ecosystem-ci bot commented Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

patak-cat commented Dec 18, 2022

Uh oh!

csr632 commented Dec 18, 2022

Uh oh!

csr632 commented Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csr632 commented Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csr632 commented Dec 18, 2022

Uh oh!

patak-cat commented Dec 18, 2022

Uh oh!

patak-cat commented Dec 18, 2022

Uh oh!

vite-ecosystem-ci bot commented Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csr632 commented Dec 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluwy commented Dec 20, 2022

Uh oh!

csr632 commented Dec 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluwy commented Dec 27, 2022

Uh oh!

csr632 Jan 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csr632 Jan 8, 2023

Choose a reason for hiding this comment

Uh oh!

csr632 commented Jan 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csr632 commented Jan 26, 2023

Uh oh!

patak-cat commented Jan 26, 2023

Uh oh!

vite-ecosystem-ci bot commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patak-cat commented Jan 26, 2023

Uh oh!

bluwy commented Jan 26, 2023

Uh oh!

patak-cat commented Mar 6, 2023

Uh oh!

bluwy commented Mar 16, 2023

Uh oh!

bluwy commented Apr 7, 2023

Uh oh!

Reviewers

Assignees

csr632 commented Dec 18, 2022 •

edited

Loading

csr632 Dec 18, 2022 •

edited

Loading

csr632 commented Dec 18, 2022 •

edited

Loading

vite-ecosystem-ci bot commented Dec 18, 2022 •

edited

Loading

csr632 commented Dec 18, 2022 •

edited

Loading

csr632 commented Dec 18, 2022 •

edited

Loading

vite-ecosystem-ci bot commented Dec 18, 2022 •

edited

Loading

csr632 commented Dec 18, 2022 •

edited

Loading

csr632 commented Dec 25, 2022 •

edited

Loading

csr632 Jan 8, 2023 •

edited

Loading

csr632 commented Jan 8, 2023 •

edited

Loading

vite-ecosystem-ci bot commented Jan 26, 2023 •

edited

Loading