-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
fix: respect resolve.conditions, when resolving browser/require field #9860
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
Conversation
| return _resolveExports(pkg, key, { | ||
| browser: targetWeb, | ||
| require: options.isRequire, | ||
| browser: targetWeb && !conditions.includes('node'), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
@antfu can you have a look? 😄 |
Description
Conditions for
browser/nodeandrequire/importare interchangable. If I, as a user, specifically requested to usenodecondition, I expect it to not usebrowsercondition. The same can be said aboutrequire.https://github.com/lukeed/resolve.exports/blob/587b0ba1adcbef257a108b7c8edb12ce0cb1bfb2/src/index.js#L72
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).