Add assetExts to ResolutionContext, remove isAssetFile

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
facebook · Mar 6, 2023 · c6548f7 · c6548f7
1 parent 6442685
commit c6548f7
Show file tree

Hide file tree

Showing 11 changed files with 44 additions and 33 deletions.
diff --git 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<string>`
+
+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<string>`
 
 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
@@ -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
@@ -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
@@ -33,10 +33,10 @@ export function createResolutionContext(
 ): $Diff<ResolutionContext, {originModulePath: string}> {
   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
@@ -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
@@ -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<Resolution, FileCandidates> {
-  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
@@ -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<string>,
   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
@@ -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<string>,
+): 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
@@ -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<string>,
+  assetExts: $ReadOnlySet<string>,
 ): 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
@@ -53,7 +53,6 @@ function getOrCreateMap<T>(
 }
 
 class DependencyGraph extends EventEmitter {
-  _assetExtensions: Set<string>;
   _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
@@ -16,7 +16,6 @@ import type {
   DoesFileExist,
   FileCandidates,
   GetRealPath,
-  IsAssetFile,
   Resolution,
   ResolveAsset,
 } from 'metro-resolver';
@@ -55,14 +54,14 @@ export type ModuleishCache<TPackage> = interface {
 };
 
 type Options<TPackage> = $ReadOnly<{
+  assetExts: $ReadOnlySet<string>,
   dirExists: DirExistsFn,
   disableHierarchicalLookup: boolean,
   doesFileExist: DoesFileExist,
   emptyModulePath: string,
   extraNodeModules: ?Object,
   getHasteModulePath: (name: string, platform: ?string) => ?string,
   getHastePackagePath: (name: string, platform: ?string) => ?string,
-  isAssetFile: IsAssetFile,
   mainFields: $ReadOnlyArray<string>,
   moduleCache: ModuleishCache<TPackage>,
   nodeModulesPaths: $ReadOnlyArray<string>,
@@ -126,10 +125,10 @@ class ModuleResolver<TPackage: Packageish> {
     resolverOptions: ResolverInputOptions,
   ): BundlerResolution {
     const {
+      assetExts,
       disableHierarchicalLookup,
       doesFileExist,
       extraNodeModules,
-      isAssetFile,
       mainFields,
       nodeModulesPaths,
       preferNativePlatform,
@@ -146,10 +145,10 @@ class ModuleResolver<TPackage: Packageish> {
       const result = Resolver.resolve(
         createDefaultContext({
           allowHaste,
+          assetExts,
           disableHierarchicalLookup,
           doesFileExist,
           extraNodeModules,
-          isAssetFile,
           mainFields,
           nodeModulesPaths,
           preferNativePlatform,