Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: handle resolve optional peer deps #9321

Merged
merged 3 commits into from
Aug 19, 2022
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jul 23, 2022

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?

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

@bluwy bluwy added the p3-significant High priority enhancement (priority) label Jul 23, 2022
@patak-dev patak-dev added this to the 3.1 milestone Jul 29, 2022
@bluwy bluwy mentioned this pull request Aug 9, 2022
9 tasks
@patak-dev patak-dev merged commit eec3886 into main Aug 19, 2022
@patak-dev patak-dev deleted the handle-optional-peer-dep branch August 19, 2022 12:55
// if so, we can resolve to a special id that errors only when imported.
if (
basedir !== root && // root has no peer dep
!isBuiltin(id) &&
Copy link
Member

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

Copy link
Member

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.

Copy link
Member Author

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
p3-significant High priority enhancement (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Bad handle of (dynamic) optional dependencies during dependency pre-bundling
3 participants