-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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: handle resolve optional peer deps #9321
Conversation
// if so, we can resolve to a special id that errors only when imported. | ||
if ( | ||
basedir !== root && // root has no peer dep | ||
!isBuiltin(id) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These id
references should be using nestedPath
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aleclarson I worry we will end up unable to keep track of these comments if you only add them to merged PRs. Would it be possible for you to create a new issue with a reproduction linking to them? Or a PR with a failing test case that others can later work on? If we don't have an open issue or PR in a few days these will be completely buried by new activity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the fix at #10593
Description
fix #6007
Introduce
__vite-optional-peer-dep
prefix (similar to `__vite-browser-external) for packages that are optional peer dependencies.Additional context
Since knowing optional peer dep requires traversing the parent for
package.json
, this may incur a small perf cost. But from what I can tell, the logic where I added is rarely called during testing.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).