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): dont overwrite isRequire from baseOptions #5872

Merged
merged 1 commit into from Nov 29, 2021

Conversation

sodatea
Copy link
Member

@sodatea sodatea commented Nov 28, 2021

Description

Fixes #5798

baseOptions may already comes with an isRequire property.
For example,

const _resolveRequire = config.createResolver({
asSrc: false,
isRequire: true
})
which in turn calls
resolvePlugin({
...resolved.resolve,
root: resolvedRoot,
isProduction,
isBuild: command === 'build',
ssrConfig: resolved.ssr,
asSrc: true,
preferRelative: false,
tryIndex: true,
...options
})

Additional context

Need to add a test case (that esbuildDepPlugin should resolve to the CJS entry of a transitive dependency if it is required) later.


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.

@Shinigami92 Shinigami92 added needs test p3-minor-bug An edge case that only affects very specific usage (priority) labels Nov 28, 2021
@Shinigami92 Shinigami92 self-requested a review November 28, 2021 14:35
@patak-dev patak-dev changed the title fix(resolve): should not overwrite isRequire from baseOptions fix(resolve): dont overwrite isRequire from baseOptions Nov 29, 2021
@patak-dev patak-dev merged commit 2b91e5a into vitejs:main Nov 29, 2021
@patak-dev
Copy link
Member

Let's merge the tests in another PR so people can start testing this while debugging the remaining regressions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs test 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.7.0-beta.2] _interopRequireDefault3 is not a function
3 participants