From c6548f7ccc5b8ad59ea98f4bd7f1f5822deec0cd Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Mon, 6 Mar 2023 04:08:42 -0800 Subject: [PATCH] Add assetExts to ResolutionContext, remove isAssetFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This commit removes the `isAssetFile` API used both for the module resolver and by `transformHelpers` — both based on `resolver.assetExts`, but the former being user-overridable in the resolver via `ResolutionContext.isAssetFile`. Motivation: - Correctness: While `ResolutionContext.isAssetFile` is respected by the resolver, the current `node-haste` implementation in Metro discards this selection, coercing the resolution to a `sourceFile`. With this change, a common `isAssetFile` implementation is now shared by both layers, leading to consistent behaviour when customised. - Simplicity: `assetExts` is sufficiently expressive for most use cases — and can be bailed out from using `ResolutionContext.resolveRequest`. Changelog: **[Breaking]** Remove `isAssetFile` from custom resolver context, add `assetExts` Reviewed By: motiz88 Differential Revision: D43735610 fbshipit-source-id: 77d4aa7e17b27b24d1fae87a162753beb1501d92 --- docs/Resolution.md | 12 ++++------ .../src/PackageExportsResolve.js | 3 ++- .../src/__tests__/package-exports-test.js | 3 +-- .../metro-resolver/src/__tests__/utils.js | 2 +- packages/metro-resolver/src/index.js | 1 - packages/metro-resolver/src/resolve.js | 3 ++- packages/metro-resolver/src/types.js | 4 +--- .../metro-resolver/src/utils/isAssetFile.js | 23 +++++++++++++++++++ packages/metro/src/lib/transformHelpers.js | 13 ++++------- .../metro/src/node-haste/DependencyGraph.js | 6 +---- .../DependencyGraph/ModuleResolution.js | 7 +++--- 11 files changed, 44 insertions(+), 33 deletions(-) create mode 100644 packages/metro-resolver/src/utils/isAssetFile.js diff --git a/docs/Resolution.md b/docs/Resolution.md index f041c6ca27..ab968d8a3f 100644 --- a/docs/Resolution.md +++ b/docs/Resolution.md @@ -62,7 +62,7 @@ These are the rules that Metro's default resolver follows. Refer to [`metro-reso 2. Otherwise, try to resolve _moduleName_ as a relative or absolute path: 1. If the path is relative, convert it to an absolute path by prepending the current directory (i.e. parent of [`context.originModulePath`](#originmodulepath-string)). - 2. If the path refers to an [asset](#isassetfile-string--boolean): + 2. If the path refers to an [asset](#assetexts-readonlysetstring): 1. Use [`context.resolveAsset`](#resolveasset-dirpath-string-assetname-string-extension-string--readonlyarraystring) to collect all asset variants. 2. Return an [asset resolution](#asset-files) containing the collected asset paths. @@ -120,18 +120,16 @@ These are the rules that Metro's default resolver follows. Refer to [`metro-reso ### Resolution context +#### `assetExts: $ReadOnlySet` + +The set of file extensions used to identify asset files. Defaults to [`resolver.assetExts`](./Configuration.md#assetexts). + #### `doesFileExist: string => boolean` Returns `true` if the file with the given path exists, or `false` otherwise. By default, Metro implements this by consulting an in-memory map of the filesystem that has been prepared in advance. This approach avoids disk I/O during module resolution. -#### `isAssetFile: string => boolean` - -Returns `true` if the given path represents an asset file, or `false` otherwise. - -By default, Metro implements this by checking the file's extension against [`resolver.assetExts`](./Configuration.md#assetexts). - #### `nodeModulesPaths: $ReadOnlyArray` A list of paths to check for modules after looking through all `node_modules` directories. diff --git a/packages/metro-resolver/src/PackageExportsResolve.js b/packages/metro-resolver/src/PackageExportsResolve.js index 1953131964..f2d60c8e30 100644 --- a/packages/metro-resolver/src/PackageExportsResolve.js +++ b/packages/metro-resolver/src/PackageExportsResolve.js @@ -16,6 +16,7 @@ import InvalidModuleSpecifierError from './errors/InvalidModuleSpecifierError'; import InvalidPackageConfigurationError from './errors/InvalidPackageConfigurationError'; import PackagePathNotExportedError from './errors/PackagePathNotExportedError'; import resolveAsset from './resolveAsset'; +import isAssetFile from './utils/isAssetFile'; import toPosixPath from './utils/toPosixPath'; /** @@ -99,7 +100,7 @@ export function resolvePackageTargetFromExports( patternMatch != null ? target.replace('*', patternMatch) : target, ); - if (context.isAssetFile(filePath)) { + if (isAssetFile(filePath, context.assetExts)) { const assetResult = resolveAsset(context, filePath); if (assetResult != null) { diff --git a/packages/metro-resolver/src/__tests__/package-exports-test.js b/packages/metro-resolver/src/__tests__/package-exports-test.js index 9a92e9abce..0e04bbeef4 100644 --- a/packages/metro-resolver/src/__tests__/package-exports-test.js +++ b/packages/metro-resolver/src/__tests__/package-exports-test.js @@ -771,7 +771,6 @@ describe('with package exports resolution enabled', () => { describe('asset resolutions', () => { const assetResolutions = ['1', '1.5', '2', '3', '4']; - const isAssetFile = (filePath: string) => filePath.endsWith('.png'); const baseContext = { ...createResolutionContext({ @@ -786,7 +785,7 @@ describe('with package exports resolution enabled', () => { '/root/node_modules/test-pkg/assets/icons/metro@2x.png': '', '/root/node_modules/test-pkg/assets/icons/metro@3x.png': '', }), - isAssetFile, + assetExts: new Set(['png']), originModulePath: '/root/src/main.js', unstable_enablePackageExports: true, }; diff --git a/packages/metro-resolver/src/__tests__/utils.js b/packages/metro-resolver/src/__tests__/utils.js index bea0d2628e..b98cfaad3c 100644 --- a/packages/metro-resolver/src/__tests__/utils.js +++ b/packages/metro-resolver/src/__tests__/utils.js @@ -33,10 +33,10 @@ export function createResolutionContext( ): $Diff { return { allowHaste: true, + assetExts: new Set(['jpg', 'png']), customResolverOptions: {}, disableHierarchicalLookup: false, extraNodeModules: null, - isAssetFile: () => false, mainFields: ['browser', 'main'], nodeModulesPaths: [], preferNativePlatform: false, diff --git a/packages/metro-resolver/src/index.js b/packages/metro-resolver/src/index.js index 5c88aef6b8..bfd375d824 100644 --- a/packages/metro-resolver/src/index.js +++ b/packages/metro-resolver/src/index.js @@ -21,7 +21,6 @@ export type { FileCandidates, FileResolution, GetRealPath, - IsAssetFile, ResolutionContext, Resolution, ResolveAsset, diff --git a/packages/metro-resolver/src/resolve.js b/packages/metro-resolver/src/resolve.js index f4f42b8e0c..92acd3cc7f 100644 --- a/packages/metro-resolver/src/resolve.js +++ b/packages/metro-resolver/src/resolve.js @@ -30,6 +30,7 @@ import formatFileCandidates from './errors/formatFileCandidates'; import {getPackageEntryPoint} from './PackageResolve'; import {resolvePackageTargetFromExports} from './PackageExportsResolve'; import resolveAsset from './resolveAsset'; +import isAssetFile from './utils/isAssetFile'; function resolve( context: ResolutionContext, @@ -369,7 +370,7 @@ function resolveFile( fileName: string, platform: string | null, ): Result { - if (context.isAssetFile(fileName)) { + if (isAssetFile(fileName, context.assetExts)) { const assetResolutions = resolveAsset( context, path.join(dirPath, fileName), diff --git a/packages/metro-resolver/src/types.js b/packages/metro-resolver/src/types.js index b40418171c..b5ad29ab0a 100644 --- a/packages/metro-resolver/src/types.js +++ b/packages/metro-resolver/src/types.js @@ -71,7 +71,6 @@ export type PackageInfo = $ReadOnly<{ */ export type DoesFileExist = (filePath: string) => boolean; export type GetRealPath = (path: string) => ?string; -export type IsAssetFile = (fileName: string) => boolean; /** * Given a directory path and the base asset name, return a list of all the @@ -87,6 +86,7 @@ export type ResolveAsset = ( export type ResolutionContext = $ReadOnly<{ allowHaste: boolean, + assetExts: $ReadOnlySet, customResolverOptions: CustomResolverOptions, disableHierarchicalLookup: boolean, doesFileExist: DoesFileExist, @@ -103,8 +103,6 @@ export type ResolutionContext = $ReadOnly<{ */ getPackageForModule: (modulePath: string) => ?PackageInfo, - isAssetFile: IsAssetFile, - /** * The ordered list of fields to read in `package.json` to resolve a main * entry point based on the "browser" field spec. diff --git a/packages/metro-resolver/src/utils/isAssetFile.js b/packages/metro-resolver/src/utils/isAssetFile.js new file mode 100644 index 0000000000..725d724c1e --- /dev/null +++ b/packages/metro-resolver/src/utils/isAssetFile.js @@ -0,0 +1,23 @@ +/** + * 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 + */ + +import path from 'path'; + +/** + * Determine if a file path should be considered an asset file based on the + * given `assetExts`. + */ +export default function isAssetFile( + filePath: string, + assetExts: $ReadOnlySet, +): boolean { + return assetExts.has(path.extname(filePath).slice(1)); +} diff --git a/packages/metro/src/lib/transformHelpers.js b/packages/metro/src/lib/transformHelpers.js index c3c2e7c1b4..9e2b70c926 100644 --- a/packages/metro/src/lib/transformHelpers.js +++ b/packages/metro/src/lib/transformHelpers.js @@ -23,8 +23,8 @@ import type {Type} from 'metro-transform-worker'; import type {RequireContext} from './contextModule'; import {getContextModuleTemplate} from './contextModuleTemplates'; +import isAssetFile from 'metro-resolver/src/utils/isAssetFile'; -const path = require('path'); import type {ResolverInputOptions} from '../shared/types.flow'; type InlineRequiresRaw = {+blockList: {[string]: true, ...}, ...} | boolean; @@ -143,6 +143,7 @@ async function getTransformFn( options, resolverOptions, ); + const assetExts = new Set(config.resolver.assetExts); return async (modulePath: string, requireContext: ?RequireContext) => { let templateBuffer: Buffer; @@ -173,11 +174,7 @@ async function getTransformFn( modulePath, { ...transformOptions, - type: getType( - transformOptions.type, - modulePath, - config.resolver.assetExts, - ), + type: getType(transformOptions.type, modulePath, assetExts), inlineRequires: removeInlineRequiresBlockListFromOptions( modulePath, inlineRequires, @@ -191,13 +188,13 @@ async function getTransformFn( function getType( type: string, filePath: string, - assetExts: $ReadOnlyArray, + assetExts: $ReadOnlySet, ): Type { if (type === 'script') { return type; } - if (assetExts.indexOf(path.extname(filePath).slice(1)) !== -1) { + if (isAssetFile(filePath, assetExts)) { return 'asset'; } diff --git a/packages/metro/src/node-haste/DependencyGraph.js b/packages/metro/src/node-haste/DependencyGraph.js index c106194877..db1b672153 100644 --- a/packages/metro/src/node-haste/DependencyGraph.js +++ b/packages/metro/src/node-haste/DependencyGraph.js @@ -53,7 +53,6 @@ function getOrCreateMap( } class DependencyGraph extends EventEmitter { - _assetExtensions: Set; _config: ConfigT; _haste: MetroFileMap; _fileSystem: FileSystem; @@ -89,9 +88,6 @@ class DependencyGraph extends EventEmitter { super(); this._config = config; - this._assetExtensions = new Set( - config.resolver.assetExts.map(asset => '.' + asset), - ); const {hasReducedPerformance, watch} = options ?? {}; const initializingMetroLogEntry = log( @@ -176,6 +172,7 @@ class DependencyGraph extends EventEmitter { _createModuleResolver() { this._moduleResolver = new ModuleResolver({ + assetExts: new Set(this._config.resolver.assetExts), dirExists: (filePath: string) => { try { return fs.lstatSync(filePath).isDirectory(); @@ -191,7 +188,6 @@ class DependencyGraph extends EventEmitter { this._hasteModuleMap.getModule(name, platform, true), getHastePackagePath: (name, platform) => this._hasteModuleMap.getPackage(name, platform, true), - isAssetFile: file => this._assetExtensions.has(path.extname(file)), mainFields: this._config.resolver.resolverMainFields, moduleCache: this._moduleCache, nodeModulesPaths: this._config.resolver.nodeModulesPaths, diff --git a/packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js b/packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js index 3c31ab49c0..f727d1bebf 100644 --- a/packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js +++ b/packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js @@ -16,7 +16,6 @@ import type { DoesFileExist, FileCandidates, GetRealPath, - IsAssetFile, Resolution, ResolveAsset, } from 'metro-resolver'; @@ -55,6 +54,7 @@ export type ModuleishCache = interface { }; type Options = $ReadOnly<{ + assetExts: $ReadOnlySet, dirExists: DirExistsFn, disableHierarchicalLookup: boolean, doesFileExist: DoesFileExist, @@ -62,7 +62,6 @@ type Options = $ReadOnly<{ extraNodeModules: ?Object, getHasteModulePath: (name: string, platform: ?string) => ?string, getHastePackagePath: (name: string, platform: ?string) => ?string, - isAssetFile: IsAssetFile, mainFields: $ReadOnlyArray, moduleCache: ModuleishCache, nodeModulesPaths: $ReadOnlyArray, @@ -126,10 +125,10 @@ class ModuleResolver { resolverOptions: ResolverInputOptions, ): BundlerResolution { const { + assetExts, disableHierarchicalLookup, doesFileExist, extraNodeModules, - isAssetFile, mainFields, nodeModulesPaths, preferNativePlatform, @@ -146,10 +145,10 @@ class ModuleResolver { const result = Resolver.resolve( createDefaultContext({ allowHaste, + assetExts, disableHierarchicalLookup, doesFileExist, extraNodeModules, - isAssetFile, mainFields, nodeModulesPaths, preferNativePlatform,