Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove extra path shorten when resolving from a dir #13381

Merged
merged 2 commits into from Jun 6, 2023

Conversation

fi3ework
Copy link
Member

Description

fix #13320

When resolving nested packages, path.dirname inadvertently shortens the initial resolving path. Although pnpm tolerates this due to its topological structure, Yarn3 does not. To fix this, the proposed PR prevents path.dirname from being invoked when the importer is already a directory.

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.

@stackblitz
Copy link

stackblitz bot commented May 30, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev
Copy link
Member

Should we guard against importer being a directory? Is it valid to pass to resolve a dir as importer? I would imagine we would need checks in other places too if that is valid. I wonder if we should fix the resolve call referenced here: #13320 (comment), adding a generic /index.js to the basedir used as importer. mmmm... looks like we can't because of this existence check. I'm fine applying this as a temporal patch (adding a comment about why is needed), but I think we may be treating a symptom here.

@bluwy
Copy link
Member

bluwy commented May 31, 2023

I think it's better to fix the importer path too, something like how CSS does currently should be good enough:

path.join(basedir, '*'),

so it's handled properly in the resolver.

@bluwy bluwy added the p3-minor-bug An edge case that only affects very specific usage (priority) label May 31, 2023
@fi3ework
Copy link
Member Author

Agree it's better to prevent resolving from dir if possible. IIUC, maybe we could join package.json with basedir here

return await resolve(nestedPath, basedir, undefined, ssr)
to fix this issue as we are pretty sure basedir is the directory of package.json of dep to optimize?

@patak-dev
Copy link
Member

Interesting, resolving as /package.json is a clever trick. I wonder if it may not confuse us more though compared to using the more explicit path.join(basedir, '*') that we know also works. @bluwy what do you think? We could try any of them during the 4.4 beta.

@patak-dev patak-dev added this to the 4.4 milestone Jun 1, 2023
@bluwy
Copy link
Member

bluwy commented Jun 2, 2023

I think using /package.json sounds good too, since we know that exists 👍

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jun 6, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ❌ failure
nuxt ❌ failure
nx ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ❌ failure
sveltekit ✅ success
unocss ✅ success
vite-plugin-pwa ✅ 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

@patak-dev patak-dev merged commit 5503198 into vitejs:main Jun 6, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite is unable to resolve nested dependency in optimizeDeps.include
3 participants