Skip to content

Commit

Permalink
Add warning for invalid "exports" targets and import specifier "expor…
Browse files Browse the repository at this point in the history 
…ts" pattern matches

Summary:
Implements checks to validate allowed subpath segments during `"exports"` resolution, when `unstable_enablePackageExports` is set.

Changelog: [Internal]

Reviewed By: motiz88

Differential Revision: D43348624

fbshipit-source-id: a37dc0e182ef4a86fee86ffe5649e9a602e3476d
  • Loading branch information
@huntie @facebook-github-bot
huntie authored and facebook-github-bot committed Feb 17, 2023
1 parent 0ecd442 commit 4785644
Show file tree
Hide file tree
Showing 4 changed files with 151 additions and 11 deletions.
65 changes: 55 additions & 10 deletions packages/metro-resolver/src/PackageExportsResolve.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import type {ExportMap, FileResolution, ResolutionContext} from './types';

import path from 'path';
import InvalidModuleSpecifierError from './errors/InvalidModuleSpecifierError';
import InvalidPackageConfigurationError from './errors/InvalidPackageConfigurationError';
import PackagePathNotExportedError from './errors/PackagePathNotExportedError';
import resolveAsset from './resolveAsset';
Expand All @@ -28,6 +29,8 @@ import toPosixPath from './utils/toPosixPath';
*
* @throws {InvalidPackageConfigurationError} Raised if configuration specified
* by `exportsField` is invalid.
* @throws {InvalidModuleSpecifierError} Raised if the resolved module specifier
* is invalid.
* @throws {PackagePathNotExportedError} Raised when the requested subpath is
* not exported.
*/
Expand Down Expand Up @@ -62,16 +65,39 @@ export function resolvePackageTargetFromExports(
);
}

const match = matchSubpathFromExports(
const {target, patternMatch} = matchSubpathFromExports(
context,
subpath,
exportMap,
platform,
raiseConfigError,
);

if (match != null) {
const filePath = path.join(packagePath, match);
if (target != null) {
const invalidSegmentInTarget = findInvalidPathSegment(target.slice(2));

if (invalidSegmentInTarget != null) {
raiseConfigError(
`The target for "${subpath}" defined in "exports" is "${target}", ` +
'however this value is an invalid subpath or subpath pattern ' +
`because it includes "${invalidSegmentInTarget}".`,
);
}

if (patternMatch != null && findInvalidPathSegment(patternMatch) != null) {
throw new InvalidModuleSpecifierError({
importSpecifier: modulePath,
reason:
`The target for "${subpath}" defined in "exports" is "${target}", ` +
'however this expands to an invalid subpath because the pattern ' +
`match "${patternMatch}" is invalid.`,
});
}

const filePath = path.join(
packagePath,
patternMatch != null ? target.replace('*', patternMatch) : target,
);

if (context.isAssetFile(filePath)) {
const assetResult = resolveAsset(context, filePath);
Expand Down Expand Up @@ -134,7 +160,7 @@ function normalizeExportsField(
if (subpathKeys.length !== 0) {
raiseConfigError(
'The "exports" field cannot have keys which are both subpaths and ' +
'condition names at the same level',
'condition names at the same level.',
);
}

Expand Down Expand Up @@ -183,7 +209,10 @@ function matchSubpathFromExports(
exportMap: ExportMap,
platform: string | null,
raiseConfigError: (reason: string) => void,
): ?string {
): $ReadOnly<{
target: string | null,
patternMatch: string | null,
}> {
const conditionNames = new Set([
'default',
...context.unstable_conditionNames,
Expand All @@ -198,10 +227,11 @@ function matchSubpathFromExports(
raiseConfigError,
);

let match = exportMapAfterConditions[subpath];
let target = exportMapAfterConditions[subpath];
let patternMatch = null;

// Attempt to match after expanding any subpath pattern keys
if (match == null) {
if (target == null) {
// Gather keys which are subpath patterns in descending order of specificity
const expansionKeys = Object.keys(exportMapAfterConditions)
.filter(key => key.includes('*'))
Expand All @@ -216,16 +246,16 @@ function matchSubpathFromExports(
break;
}

const patternMatch = matchSubpathPattern(key, subpath);
patternMatch = matchSubpathPattern(key, subpath);

if (patternMatch != null) {
match = value == null ? null : value.replace('*', patternMatch);
target = value;
break;
}
}
}

return match;
return {target, patternMatch};
}

type FlattenedExportMap = $ReadOnly<{[subpath: string]: string | null}>;
Expand Down Expand Up @@ -321,3 +351,18 @@ function matchSubpathPattern(

return null;
}

function findInvalidPathSegment(subpath: string): ?string {
for (const segment of subpath.split(/[\\/]/)) {
if (
segment === '' ||
segment === '.' ||
segment === '..' ||
segment === 'node_modules'
) {
return segment;
}
}

return null;
}
55 changes: 55 additions & 0 deletions packages/metro-resolver/src/__tests__/package-exports-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ describe('with package exports resolution enabled', () => {
exports: {
'.': './index.js',
'./foo.js': './lib/foo.js',
'./baz': './node_modules/baz/index.js',
},
}),
'/root/node_modules/test-pkg/index.js': '',
Expand All @@ -251,6 +252,7 @@ describe('with package exports resolution enabled', () => {
'/root/node_modules/test-pkg/lib/foo.js.js': '',
'/root/node_modules/test-pkg/lib/foo.ios.js': '',
'/root/node_modules/test-pkg/private/bar.js': '',
'/root/node_modules/test-pkg/node_modules/baz/index.js': '',
}),
originModulePath: '/root/src/main.js',
unstable_enablePackageExports: true,
Expand Down Expand Up @@ -280,6 +282,29 @@ describe('with package exports resolution enabled', () => {
);
});

test('[nonstrict] should fall back and log warning for an invalid "exports" target value', () => {
const logWarning = jest.fn();
const context = {
...baseContext,
unstable_logWarning: logWarning,
};

// TODO(T145206395): Improve this error trace
expect(() => Resolver.resolve(context, 'test-pkg/baz', null))
.toThrowErrorMatchingInlineSnapshot(`
"Module does not exist in the Haste module map or in these directories:
/root/src/node_modules
/root/node_modules
/node_modules
"
`);
expect(logWarning).toHaveBeenCalledTimes(1);
expect(logWarning.mock.calls[0][0]).toMatchInlineSnapshot(`
"The package /root/node_modules/test-pkg contains an invalid package.json configuration. Consider raising this issue with the package maintainer(s).
Reason: The target for \\"./baz\\" defined in \\"exports\\" is \\"./node_modules/baz/index.js\\", however this value is an invalid subpath or subpath pattern because it includes \\"node_modules\\". Falling back to file-based resolution."
`);
});

describe('should resolve "exports" target directly', () => {
test('without expanding `sourceExts`', () => {
expect(Resolver.resolve(baseContext, 'test-pkg/foo.js', null)).toEqual({
Expand Down Expand Up @@ -360,6 +385,8 @@ describe('with package exports resolution enabled', () => {
'/root/node_modules/test-pkg/src/features/foo.js.js': '',
'/root/node_modules/test-pkg/src/features/bar/Bar.js': '',
'/root/node_modules/test-pkg/src/features/baz.native.js': '',
'/root/node_modules/test-pkg/src/features/node_modules/foo/index.js':
'',
'/root/node_modules/test-pkg/assets/Logo.js': '',
}),
originModulePath: '/root/src/main.js',
Expand Down Expand Up @@ -415,6 +442,34 @@ describe('with package exports resolution enabled', () => {
`);
});

test('[nonstrict] should fall back and log warning for an invalid pattern match substitution', () => {
const logWarning = jest.fn();
const context = {
...baseContext,
unstable_logWarning: logWarning,
};

// TODO(T145206395): Improve this error trace
expect(() =>
Resolver.resolve(
context,
'test-pkg/features/node_modules/foo/index.js',
null,
),
).toThrowErrorMatchingInlineSnapshot(`
"Module does not exist in the Haste module map or in these directories:
/root/src/node_modules
/root/node_modules
/node_modules
"
`);
expect(logWarning).toHaveBeenCalledTimes(1);
expect(logWarning.mock.calls[0][0]).toMatchInlineSnapshot(`
"Invalid import specifier /root/node_modules/test-pkg/features/node_modules/foo/index.js.
Reason: The target for \\"./features/node_modules/foo/index.js\\" defined in \\"exports\\" is \\"./src/features/*.js\\", however this expands to an invalid subpath because the pattern match \\"node_modules/foo/index\\" is invalid. Falling back to file-based resolution."
`);
});

describe('package encapsulation', () => {
test('[nonstrict] should fall back to "browser" spec resolution and log inaccessible import warning', () => {
const logWarning = jest.fn();
Expand Down
36 changes: 36 additions & 0 deletions packages/metro-resolver/src/errors/InvalidModuleSpecifierError.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow strict
* @format
* @oncall react_native
*/

export default class InvalidModuleSpecifierError extends Error {
/**
* Either the import specifier read, or the absolute path of the module being
* resolved (used when import specifier is externally remapped).
*/
importSpecifier: string;

/**
* The description of the error cause.
*/
reason: string;

constructor(
opts: $ReadOnly<{
importSpecifier: string,
reason: string,
}>,
) {
super(
`Invalid import specifier ${opts.importSpecifier}.\nReason: ` +
opts.reason,
);
Object.assign(this, opts);
}
}
6 changes: 5 additions & 1 deletion packages/metro-resolver/src/resolve.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import isAbsolutePath from 'absolute-path';
import path from 'path';
import FailedToResolveNameError from './errors/FailedToResolveNameError';
import FailedToResolvePathError from './errors/FailedToResolvePathError';
import InvalidModuleSpecifierError from './errors/InvalidModuleSpecifierError';
import InvalidPackageConfigurationError from './errors/InvalidPackageConfigurationError';
import InvalidPackageError from './errors/InvalidPackageError';
import PackagePathNotExportedError from './errors/PackagePathNotExportedError';
Expand Down Expand Up @@ -265,7 +266,10 @@ function resolvePackage(
' Falling back to file-based resolution. Consider updating the ' +
'call site or asking the package maintainer(s) to expose this API.',
);
} else if (e instanceof InvalidPackageConfigurationError) {
} else if (
e instanceof InvalidModuleSpecifierError ||
e instanceof InvalidPackageConfigurationError
) {
context.unstable_logWarning(
e.message + ' Falling back to file-based resolution.',
);
Expand Down

0 comments on commit 4785644

Please sign in to comment.