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.

Sign up for GitHub

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

Jump to bottom

fix: respect resolve.conditions, when resolving browser/require field #9860

Merged
merged 1 commit into from Sep 22, 2022
Merged

fix: respect resolve.conditions, when resolving browser/require field #9860

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/vite/src/node/plugins/resolve.ts
View file
Open in desktop
Expand Up @@ -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

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.

require: options.isRequire && !conditions.includes('import'),
conditions
})
}
Expand Down