Skip to content

fix: wrongly resolve to optimized doppelganger#11290

Merged
patak-cat merged 7 commits intovitejs:mainfrom
csr632:resolve-optimized-dup-deps
Dec 12, 2022
Merged

fix: wrongly resolve to optimized doppelganger#11290
patak-cat merged 7 commits intovitejs:mainfrom
csr632:resolve-optimized-dup-deps

Conversation

@csr632
Copy link
Member

@csr632 csr632 commented Dec 9, 2022 •
edited
Loading

Description

This PR fix a bug:
when a dependency has a doppelganger (duplicate package with same name but different version), and one of it is optimized, then some dependents of the package wrongly get the optimized version(expect unoptimized version).

Example:

- project
  - test-package-b@1.0.0 (optimized)
  - test-package-a (not optimized)
    - test-package-b@2.0.0

When test-package-a import test-package-b, it expects test-package-b@2.0.0. But it actually get test-package-b@1.0.0 (the optimized bundle).
It should resolve to the nested dependency, instead of the optimized doppelganger.

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

@csr632
Copy link
Member Author

csr632 commented Dec 10, 2022 •
edited
Loading

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

Line 879 in 8d05daf

return depsOptimizer.getOptimizedDepId(depInfo)

I think the wrong result is return by this line. It return the optimized bundle without checking whether it is resolveable from theimporter.

@csr632 csr632 changed the title test: wrongly resolve to optimized doppelganger fix: wrongly resolve to optimized doppelganger Dec 10, 2022
@csr632
Copy link
Member Author

csr632 commented Dec 10, 2022 •
edited
Loading

I just push a commit to fix it.
@patak-dev @bluwy Could you review this when you have time?

csr632
csr632 commented Dec 10, 2022
View reviewed changes
@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) feat: deps optimizer Esbuild Dependencies Optimization labels Dec 10, 2022
bluwy
bluwy reviewed Dec 10, 2022
View reviewed changes
csr632
csr632 commented Dec 10, 2022
View reviewed changes
@csr632 csr632 requested a review from bluwy December 11, 2022 10:15
bluwy
bluwy reviewed Dec 11, 2022
View reviewed changes
@csr632 csr632 requested a review from bluwy December 12, 2022 13:09
@patak-cat
Copy link
Member

patak-cat commented Dec 12, 2022

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 12, 2022 •
edited
Loading

📝 Ran ecosystem CI: Open

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

@patak-cat
Copy link
Member

patak-cat commented Dec 12, 2022

/ecosystem-ci run nuxt-framework

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 12, 2022 •
edited
Loading

📝 Ran ecosystem CI: Open

suite result
nuxt-framework ✅ success

@patak-cat
Copy link
Member

patak-cat commented Dec 12, 2022

/ecosystem-ci run sveltekit

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 12, 2022 •
edited
Loading

📝 Ran ecosystem CI: Open

suite result
sveltekit ❌ failure

@patak-cat
Copy link
Member

patak-cat commented Dec 12, 2022

/ecosystem-ci run sveltekit

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 12, 2022 •
edited
Loading

📝 Ran ecosystem CI: Open

suite result
sveltekit ✅ success

@patak-cat patak-cat merged commit 34fec41 into vitejs:main Dec 12, 2022
@ryansolid ryansolid mentioned this pull request Dec 18, 2022
Closed
@csr632 csr632 mentioned this pull request Dec 18, 2022
Closed
9 tasks
patak-cat added a commit that referenced this pull request Dec 18, 2022
@patak-cat
fix: revert #11290
51d23e6 
This reverts commit 34fec41.
@patak-cat patak-cat mentioned this pull request Dec 18, 2022
Merged
4 tasks
patak-cat added a commit that referenced this pull request Dec 18, 2022
@patak-cat
fix: revert #11290 (#11412)
6587d2f
csr632 added a commit to csr632/vite that referenced this pull request Dec 18, 2022
@csr632
fix: wrongly resolve to optimized doppelganger (vitejs#11290)
46ac0b7
csr632 added a commit to csr632/vite that referenced this pull request Jan 5, 2023
@csr632
fix: wrongly resolve to optimized doppelganger (vitejs#11290)
82ad4bf
@csr632
Copy link
Member Author

csr632 commented Jan 26, 2023

This is reworked in #11410

futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
@csr632 @futurGH
fix: wrongly resolve to optimized doppelganger (vitejs#11290)
279ca74
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
@patak-cat @futurGH
fix: revert vitejs#11290 (vitejs#11412)
9a5b2b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bluwy bluwy bluwy approved these changes

Assignees

No one assigned

Labels

feat: deps optimizer Esbuild Dependencies Optimization p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@csr632 @patak-cat @bluwy