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: respect resolve.conditions, when resolving browser/require field #9860

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Aug 26, 2022

Description

Conditions for browser/node and require/import are interchangable. If I, as a user, specifically requested to use node condition, I expect it to not use browser condition. The same can be said about require.

https://github.com/lukeed/resolve.exports/blob/587b0ba1adcbef257a108b7c8edb12ce0cb1bfb2/src/index.js#L72

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

@@ -984,8 +984,8 @@ function resolveExports(
}

return _resolveExports(pkg, key, {
browser: targetWeb,
require: options.isRequire,
browser: targetWeb && !conditions.includes('node'),
Copy link
Member

Choose a reason for hiding this comment

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

Is there a use case for this one? Maybe we should warn here? For targetWeb, we always need browser resolution, no?

Copy link
Member Author

@sheremet-va sheremet-va Aug 26, 2022

Choose a reason for hiding this comment

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

Currently the only usage is in Vitest, where part of the codebase is transformed with ssr flag, and part is not. Unfortunately it mixes up resolving, where some packages are resolved with browser condition, and others are with node condition, because resolve.exports always adds one or the other.

We do different transformations for performance reasons. Also, usually, frameworks are transformed without ssr flag (like, Vue), because they might try to inject Node logic, or redirect file to noop, because they expect some code to not be run (like with onMounted in Svelte).

I don't see anything wrong with adding this check here, even if not considering usecases above. Otherwise it creates ambiguity, if user injected 'node' condition (why would it use 'browser'?). There is no way of opting out. Currently, in Vitest injecting node condition helps with the packages that defined node above all other export conditions, but fails otherwise.

Also I don't think we need to come up with another solution for this (like, "web"-like mode, but for...). Vitest has unique requirements where code should be transformed as for the web, but files should be resolved as for node (because we are running code inside Node, and not in a browser, and we prefer its resolution algorithm).

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, more looking at what the linked resolve.exports does.
Shouldn't isRequire already have !conditions.includes('import') into account though? We are forcing it to be true in other places. I'm fine moving forward with this change for now, since as you explained if the user is adding this condition, it doesn't hurt to respect it here.

@patak-dev patak-dev requested a review from antfu August 30, 2022 21:42
@sheremet-va
Copy link
Member Author

@antfu can you have a look? 😄

@patak-dev patak-dev added this to the 3.2 milestone Sep 12, 2022
@patak-dev patak-dev added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Sep 12, 2022
@patak-dev patak-dev merged commit 9a83eaf into vitejs:main Sep 22, 2022
@sheremet-va sheremet-va deleted the fix/respect-conditions branch September 22, 2022 09:17
@sapphi-red sapphi-red mentioned this pull request Nov 14, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants