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

fix(node-resolve): fix "browser" and "exports.browser" field mappings #843

Closed
wants to merge 4 commits into from
Closed

fix(node-resolve): fix "browser" and "exports.browser" field mappings #843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94b4bdf
fix(node-resolve): fix "browser" field mappings.
DorianBlues Feb 1, 2021
93bb025
fix(node-resolve): add test.
DorianBlues Feb 1, 2021
756a6e8
fix(node-resolve): check for exports.browser field
Mar 24, 2021
02d6d25
fix(node-resolve): add test for exports.browser feature
Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/node-resolve/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export default {
throw new Error(warning);
},
output: [
{ file: pkg.main, format: 'cjs', exports: 'named' },
{ file: pkg.main, format: 'cjs', exports: 'named', sourcemap: true },
{ file: pkg.module, format: 'es', plugins: [emitModulePackageFile()] }
]
};
17 changes: 16 additions & 1 deletion packages/node-resolve/src/package/resolvePackageTarget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,25 @@ async function resolvePackageTarget(context, { target, subpath, pattern, interna
}

if (target && typeof target === 'object') {
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;
}
}
}
}
Comment on lines +89 to +102
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.

subpath,
pattern,
internal
Expand Down
7 changes: 6 additions & 1 deletion packages/node-resolve/src/resolveImportSpecifiers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ async function resolveId({
importer,
moduleDirs: moduleDirectories,
conditions: exportConditions,
useBrowserOverrides,
packageBrowserField,
resolveId(id, parent) {
return resolveId({
importSpecifier: id,
Expand All @@ -103,6 +105,7 @@ async function resolveId({
mainFields,
preserveSymlinks,
useBrowserOverrides,
packageBrowserField,
baseDir,
moduleDirectories
});
Expand All @@ -127,7 +130,9 @@ async function resolveId({
moduleDirs: moduleDirectories,
pkgURL,
pkgJsonPath,
conditions: exportConditions
conditions: exportConditions,
useBrowserOverrides,
packageBrowserField
};
const resolvedPackageExport = await resolvePackageExports(
context,
Expand Down
30 changes: 30 additions & 0 deletions packages/node-resolve/test/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,33 @@ test('pkg.browser with mapping to prevent bundle by specifying a value of false'

t.is(module.exports, 'ok');
});

test('pkg.browser can override the export map result', async (t) => {
const bundle = await rollup({
input: 'browser-override-exports.js',
plugins: [nodeResolve({ browser: true }), commonjs()]
});
const { module } = await testBundle(t, bundle);

t.is(module.exports, 'browser');
});

test('exports.browser takes precedence over export map result, when browser:true', async (t) => {
const bundle = await rollup({
input: 'browser-exports-browser.js',
plugins: [nodeResolve({ browser: true }), commonjs()]
});
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.

});

test('exports.browser does not take precedence over export map result, when browser:false', async (t) => {
const bundle = await rollup({
input: 'browser-exports-browser.js',
plugins: [nodeResolve(), commonjs()]
});
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 •
edited
Loading

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.

});
3 changes: 3 additions & 0 deletions packages/node-resolve/test/fixtures/browser-exports-browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const b = require('exports-browser');

module.exports = b;
3 changes: 3 additions & 0 deletions packages/node-resolve/test/fixtures/browser-override-exports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const Nanoid = require('nanoid');

module.exports = Nanoid;
1 change: 1 addition & 0 deletions packages/node-resolve/test/fixtures/node_modules/exports-browser/browser.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/node-resolve/test/fixtures/node_modules/exports-browser/ignored-browser.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/node-resolve/test/fixtures/node_modules/exports-browser/import.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions packages/node-resolve/test/fixtures/node_modules/exports-browser/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/node-resolve/test/fixtures/node_modules/exports-browser/require.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/node-resolve/test/fixtures/node_modules/nanoid/index.cjs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/node-resolve/test/fixtures/node_modules/nanoid/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions packages/node-resolve/test/fixtures/node_modules/nanoid/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.