Skip to content

fix(node-resolve): do not ignore exceptions #564

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

Merged
shellscape merged 11 commits into from
Oct 25, 2020
Merged

fix(node-resolve): do not ignore exceptions #564

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
39c9ce1
do not ignore errors
tjenkinson Sep 5, 2020
b456520
do not error when import specifier does not map to file
tjenkinson Sep 5, 2020
d78fa0f
always swallow MODULE_NOT_FOUND errors
tjenkinson Sep 5, 2020
81890b7
return null from resolveId early if can't resolve
tjenkinson Sep 5, 2020
edffa14
strip null byte from importer if there is one
tjenkinson Sep 5, 2020
b4daacd
fix root-dir test
tjenkinson Sep 5, 2020
b321a7d
remove try/catch
tjenkinson Sep 5, 2020
44227e4
use async exists() and realpath()
tjenkinson Oct 13, 2020
3c7b64a
Merge remote-tracking branch 'upstream/master' into do-not-ignore-exc…
tjenkinson Oct 13, 2020
26f9bd2
pretend importer is `undefined` if it starts will \0
tjenkinson Oct 13, 2020
994ab6b
return null if file not found with modulesOnly option
tjenkinson Oct 14, 2020
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
95 changes: 50 additions & 45 deletions packages/node-resolve/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
const builtins = new Set(builtinList);
const ES6_BROWSER_EMPTY = '\0node-resolve:empty.js';
const nullFn = () => null;
const deepFreeze = object => {
const deepFreeze = (object) => {
Object.freeze(object);

for (const value of Object.values(object)) {
Expand Down Expand Up @@ -100,6 +100,12 @@ export function nodeResolve(opts = {}) {
// ignore IDs with null character, these belong to other plugins
if (/\0/.test(importee)) return null;

if (/\0/.test(importer)) {
// handle cases like common-js plugin which has form
// \u0000<id>?commonjs-proxy
importer = importer.slice(1);
Copy link
Member Author

@tjenkinson tjenkinson Sep 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if there's a better way to handle this case?

If you remove this change you'll see the pkg.browser with mapping to prevent bundle by specifying a value of false test fail because the call to resolveId with the null byte is not allowed

Copy link
Member

@lukastaegert lukastaegert Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one is a really good question that got overlooked so far. I do not agree with the solution, though. Files starting with \0 are under the sole maintenance of some plugin, and we should not make assumptions about what the part behind the \0 means.
I think a valid approach might be to treat the importer like undefined, which is the value that gets passed for entry points. At least for the commonjs plugin, this should work nicely as all imports from those proxy files are fully resolved absolute paths anyway, and I would expect the same from other plugins as well.

Copy link
Member Author

@tjenkinson tjenkinson Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a valid approach might be to treat the importer like undefined, which is the value that gets passed for entry points

@lukastaegert I pushed a commit to do this

}

// strip hash and query params from import
const [withoutHash, hash] = importee.split('#');
const [importPath, params] = withoutHash.split('?');
Expand Down Expand Up @@ -219,62 +225,61 @@ export function nodeResolve(opts = {}) {
importSpecifierList.push(importee);
resolveOptions = Object.assign(resolveOptions, customResolveOptions);

try {
let resolved = await resolveImportSpecifiers(importSpecifierList, resolveOptions);
let resolved = await resolveImportSpecifiers(importSpecifierList, resolveOptions);
if (!resolved) {
return null;
}

if (resolved && packageBrowserField) {
if (Object.prototype.hasOwnProperty.call(packageBrowserField, resolved)) {
if (!packageBrowserField[resolved]) {
browserMapCache.set(resolved, packageBrowserField);
return ES6_BROWSER_EMPTY;
}
resolved = packageBrowserField[resolved];
if (packageBrowserField) {
if (Object.prototype.hasOwnProperty.call(packageBrowserField, resolved)) {
if (!packageBrowserField[resolved]) {
browserMapCache.set(resolved, packageBrowserField);
return ES6_BROWSER_EMPTY;
}
browserMapCache.set(resolved, packageBrowserField);
resolved = packageBrowserField[resolved];
}
browserMapCache.set(resolved, packageBrowserField);
}

if (hasPackageEntry && !preserveSymlinks && resolved) {
const fileExists = await exists(resolved);
if (fileExists) {
resolved = await realpath(resolved);
}
if (hasPackageEntry && !preserveSymlinks) {
const fileExists = await exists(resolved);
if (fileExists) {
resolved = await realpath(resolved);
}
}

idToPackageInfo.set(resolved, packageInfo);

if (hasPackageEntry) {
if (builtins.has(resolved) && preferBuiltins && isPreferBuiltinsSet) {
return null;
} else if (importeeIsBuiltin && preferBuiltins) {
if (!isPreferBuiltinsSet) {
this.warn(
`preferring built-in module '${importee}' over local alternative at '${resolved}', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning`
);
}
return null;
} else if (jail && resolved.indexOf(normalize(jail.trim(sep))) !== 0) {
return null;
}
}
idToPackageInfo.set(resolved, packageInfo);

if (resolved && options.modulesOnly) {
const code = await readFile(resolved, 'utf-8');
if (isModule(code)) {
return {
id: `${resolved}${importSuffix}`,
moduleSideEffects: hasModuleSideEffects(resolved)
};
if (hasPackageEntry) {
if (builtins.has(resolved) && preferBuiltins && isPreferBuiltinsSet) {
return null;
} else if (importeeIsBuiltin && preferBuiltins) {
if (!isPreferBuiltinsSet) {
this.warn(
`preferring built-in module '${importee}' over local alternative at '${resolved}', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning`
);
}
return null;
} else if (jail && resolved.indexOf(normalize(jail.trim(sep))) !== 0) {
return null;
}
}

if (options.modulesOnly) {
const code = await readFile(resolved, 'utf-8');
if (isModule(code)) {
return {
id: `${resolved}${importSuffix}`,
moduleSideEffects: hasModuleSideEffects(resolved)
};
}
const result = {
id: `${resolved}${importSuffix}`,
moduleSideEffects: hasModuleSideEffects(resolved)
};
return result;
} catch (error) {
return null;
}
const result = {
id: `${resolved}${importSuffix}`,
moduleSideEffects: hasModuleSideEffects(resolved)
};
return result;
},

load(importee) {
Expand Down
20 changes: 11 additions & 9 deletions packages/node-resolve/src/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,20 +173,22 @@ export function resolveImportSpecifiers(importSpecifierList, resolveOptions) {

return resolveId(importSpecifierList[i], resolveOptions).then((result) => {
if (!resolveOptions.preserveSymlinks) {
result = realpathSync(result);
try {
result = realpathSync(result);
} catch (e) {
// maybe the file does not exist. Might be a builtin
}
}
return result;
});
});

if (i < importSpecifierList.length - 1) {
// swallow MODULE_NOT_FOUND errors from all but the last resolution
promise = promise.catch((error) => {
if (error.code !== 'MODULE_NOT_FOUND') {
throw error;
}
});
}
// swallow MODULE_NOT_FOUND errors
promise = promise.catch((error) => {
if (error.code !== 'MODULE_NOT_FOUND') {
throw error;
}
});
}

return promise;
Expand Down
4 changes: 1 addition & 3 deletions packages/node-resolve/test/root-dir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@ const { testBundle } = require('../../../util/test');

const { nodeResolve } = require('..');

process.chdir(join(__dirname, 'fixtures', 'monorepo-dedupe', 'packages', 'package-a'));

test('deduplicates modules from the given root directory', async (t) => {
const bundle = await rollup({
input: 'index.js',
input: './packages/package-a/index.js',
Copy link
Member Author

@tjenkinson tjenkinson Sep 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was causing an error because the input could not be found. It should be relative to the rootDir right?

plugins: [
nodeResolve({
dedupe: ['react'],
Expand Down