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(node-resolve): fix "browser" and "exports.browser" field mappings #843

Closed
wants to merge 4 commits into from

Conversation

brizental
Copy link

Rollup Plugin Name: node-resolve

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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 and development conditions (#786 (comment), #786 (comment)) as I believe they are unrelated and I really just want the browser 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!

Copy link
Contributor

@guybedford guybedford left a 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.

Comment on lines +89 to +102
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;
}
}
}
}
Copy link
Contributor

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,
Copy link
Contributor

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');
Copy link
Contributor

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');
Copy link
Contributor

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.

Copy link
Author

@brizental brizental Mar 25, 2021

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 :)

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

@guybedford
Copy link
Contributor

Superceded by #866.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants