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(resolve): support submodules of optional peer deps #14489

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Conversation

decs
Copy link
Contributor

@decs decs commented Sep 27, 2023

Description

#9321 added support for resolving optional peer deps. But I hit a corner case where that wasn't resolving properly when developing TypeSchema.

@sinclair/typebox is one of the optional peer deps, but we actually import it as @sinclair/typebox/compiler. Vite's logic can't detect that @sinclair/typebox/compiler is handled by the @sinclair/typebox because it looks for the exact name on peerDependencies. So if I try to build with Vite, I get this error:

[vite]: Rollup failed to resolve import "@sinclair/typebox/compiler" from "/Users/deco/sample/node_modules/@decs/typeschema/dist/index.mjs".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
error during build:
Error: [vite]: Rollup failed to resolve import "@sinclair/typebox/compiler" from "/Users/deco/sample/node_modules/@decs/typeschema/dist/index.mjs".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
    at viteWarn (file:///Users/deco/sample/node_modules/vite/dist/node/chunks/dep-df561101.js:48142:27)
    at onRollupWarning (file:///Users/deco/sample/node_modules/vite/dist/node/chunks/dep-df561101.js:48174:9)
    at onwarn (file:///Users/deco/sample/node_modules/vite/dist/node/chunks/dep-df561101.js:47902:13)
    at file:///Users/deco/sample/node_modules/rollup/dist/es/shared/node-entry.js:24271:13
    at Object.logger [as onLog] (file:///Users/deco/sample/node_modules/rollup/dist/es/shared/node-entry.js:25945:9)
    at ModuleLoader.handleInvalidResolvedId (file:///Users/deco/sample/node_modules/rollup/dist/es/shared/node-entry.js:24857:26)
    at ModuleLoader.resolveDynamicImport (file:///Users/deco/sample/node_modules/rollup/dist/es/shared/node-entry.js:24915:58)
    at async file:///Users/deco/sample/node_modules/rollup/dist/es/shared/node-entry.js:24802:32

My proposal is to fix this error by checking for the NPM package name on peerDependencies, instead of the full module name.

Additional context

I also added a test case to avoid regressions.


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 Sep 27, 2023

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

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests!

@bluwy bluwy added the p3-minor-bug An edge case that only affects very specific usage (priority) label Oct 2, 2023
@bluwy bluwy merged commit f80ff77 into vitejs:main Oct 2, 2023
9 checks passed
@bluwy bluwy mentioned this pull request Oct 3, 2023
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.

2 participants