-
-
Notifications
You must be signed in to change notification settings - Fork 588
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(node-resolve): fix "browser" and "exports.browser" field mappings #843
Conversation
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.
Thank you @brizental for the PR. I've left some feedback below.
const { packageBrowserField, useBrowserOverrides } = context; | ||
let browserTarget = null; | ||
|
||
if (useBrowserOverrides) { | ||
if (Object.keys(target).includes('browser')) { | ||
browserTarget = target.browser; | ||
} else { | ||
for (const [, value] of Object.entries(target)) { | ||
if (packageBrowserField[value]) { | ||
browserTarget = value; | ||
} | ||
} | ||
} | ||
} |
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.
I think these mappings are supposed to apply for all resolutions not just exports targets. Eg require('./index.js')
/ import './index.js'
should also be rewritten but aren't here.
In theory such a bug fix is an extension of this bug fix, so if you'd rather do it as a follow up I'm open to that - but strictly speaking it would be incomplete without it.
for (const [key, value] of Object.entries(target)) { | ||
if (key === 'default' || context.conditions.includes(key)) { | ||
const resolved = await resolvePackageTarget(context, { | ||
target: value, | ||
target: browserTarget || value, |
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.
The browserTarget
needs to be based on the value
here using the conditions fields (so the code above that checks the browser target needs to be incide this loop to read value
. Otherwise this causes a regression in the way exports resolve to begin with. I've clarified this bug in the test case directly below.
}); | ||
const { module } = await testBundle(t, bundle); | ||
|
||
t.is(module.exports, 'browser'); |
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.
This is incorrect - the correct result is the "import"
path in "exports"
gets selected which should then return 'import'
.
The reason for this is that the exports object matches the first condition that is valid in object ordering. So the "browser"
exports value isn't selected in this test case unless it is moved to the top in the package.json.
}); | ||
const { module } = await testBundle(t, bundle); | ||
|
||
t.is(module.exports, 'require'); |
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.
Somewhat suprised to see this is returning require
by default, I wasn't aware this was the case, it might be a separate issue if not introduced by this PR (in which case the previous comment should also be require
I suppose). We should surely be defaulting to ESM for top-level imports.
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.
Maybe it's because I included the commonjs
plugin as well? That is copy-pasta, so I'll just try removing it :)
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.
I'm not sure, perhaps @LarsDenBakker can clarify what the expectation is for this case.
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.
FWIW, removing the commonjs plugin made no difference.
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.
That makes sense, it's about the top-level resolver conditions, not the transforms.
This works around rollup/plugins#843. See the documentation: https://github.com/rollup/plugins/tree/master/packages/node-resolve#exportconditions
This works around rollup/plugins#843. See the documentation: https://github.com/rollup/plugins/tree/master/packages/node-resolve#exportconditions
Superceded by #866. |
Rollup Plugin Name:
node-resolve
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
Hey folks, I encountered this error and noticed there was an open PR (#786) related to it that was getting stale. I built on top of that PR and attended to the review comments.
I did not attend to the comments related to the
production
anddevelopment
conditions (#786 (comment), #786 (comment)) as I believe they are unrelated and I really just want thebrowser
fix right now. I'd be happy to take that on as a follow up, but I would love for this or the other PR to be merged sooner, if possible.Cheers!