Move package-relative path calculation under context.getPackageForModule (#1268)

Summary:
Pull Request resolved: #1268

During resolution using `package.json#exports` or the `browser` spec, we need package-relative subpaths of resolution candidates to check against these export/redirect maps.

Currently, we derive these using (pseudo code):
```
const absolutePathOfPackageRoot = context.getPackageForModule(absolutePathOfCandidate).rootPath;
const packageRelativePath = path.relative(absolutePathOfPackageRoot, absolutePathOfCandidate);
```

However, this implicitly assumes that both paths are either real, or traverse the same set of symlinks, so that the former is a prefix of the latter. If `getPackageForModule` returned `rootPath: fs.realpath(packageRoot)`, this assumption would not hold.

Eg: if `/workspace-root/pgk-a/foo.js` imports `pkg-b/bar.js`, we will try a candidate `/workspace-root/node_modules/pkg-b/bar.js`, but `node_modles/pkg-b` may be a symlink to `/workspace-root/pkg-b`, and `path.relative(realPackageRoot, candidate)` will give `../node_modules/pkg-b/bar.js`, which will not match against an export map, though it represents the same location on the filesystem.

Instead, we move the calculation of `packageRelativePath` inside `getPackageForModule` and close to implementation of the package search, where we know that the `path.relative` call is safe under the current implementation, and allowing an alternative implementation to use an alternative mechanism.

This is in preparation for a new implementation of `getClosestPackage`, which currently dominates resolution time, using `FileSystem`, which will natively return only *real* paths.

Changelog: Internal

Reviewed By: huntie

Differential Revision: D56823091

fbshipit-source-id: 3007f1732ad2aaede5035d0c4be054304b328de9

Author: robhogan

docs/Resolution.md

@@ -210,9 +210,9 @@ The ordered list of fields in `package.json` that should be read to resolve a pa
 
 Given the path to a `package.json` file, returns the parsed file contents.
 
-#### `getPackageForModule: (modulePath: string) => ?PackageInfo` <div class="label deprecated">Deprecated</div>
+#### `getPackageForModule: (absoluteModulePath: string) => ?PackageInfo` <div class="label deprecated">Deprecated</div>
 
-Given a module path that may exist under an npm package, locates and returns the package root path and parsed `package.json` contents.
+Given a candidate absolute module path that may exist under a package, locates and returns the closest package root (working upwards from the given path, stopping at the nearest `node_modules`), parsed `package.json` contents, and the package-relative path of the given path.
 
 #### `resolveHasteModule: string => ?string`
 

packages/metro-resolver/src/PackageExportsResolve.js

@@ -51,6 +51,7 @@ export function resolvePackageTargetFromExports(
    * to a package-relative subpath for comparison.
    */
   modulePath: string,
+  packageRelativePath: string,
   exportsField: ExportsField,
   platform: string | null,
 ): FileResolution {
@@ -61,13 +62,14 @@ export function resolvePackageTargetFromExports(
     });
   };
 
-  const subpath = getExportsSubpath(packagePath, modulePath);
+  const subpath = getExportsSubpath(packageRelativePath);
   const exportMap = normalizeExportsField(exportsField, createConfigError);
 
   if (!isSubpathDefinedInExports(exportMap, subpath)) {
     throw new PackagePathNotExportedError(
       `Attempted to import the module "${modulePath}" which is not listed ` +
-        `in the "exports" of "${packagePath}".`,
+        `in the "exports" of "${packagePath}" under the requested subpath ` +
+        `"${subpath}".`,
     );
   }
 
@@ -135,9 +137,7 @@ export function resolvePackageTargetFromExports(
  * Convert a module path to the package-relative subpath key to attempt for
  * "exports" field lookup.
  */
-function getExportsSubpath(packagePath: string, modulePath: string): string {
-  const packageSubpath = path.relative(packagePath, modulePath);
-
+function getExportsSubpath(packageSubpath: string): string {
   return packageSubpath === '' ? '.' : './' + toPosixPath(packageSubpath);
 }
 

packages/metro-resolver/src/PackageResolve.js

@@ -97,9 +97,10 @@ export function redirectModulePath(
   modulePath: string,
 ): string | false {
   const {getPackageForModule, mainFields, originModulePath} = context;
+  const isModulePathAbsolute = path.isAbsolute(modulePath);
 
   const containingPackage = getPackageForModule(
-    path.isAbsolute(modulePath) ? modulePath : originModulePath,
+    isModulePathAbsolute ? modulePath : originModulePath,
   );
 
   if (containingPackage == null) {
@@ -109,11 +110,19 @@ export function redirectModulePath(
 
   let redirectedPath;
 
-  if (modulePath.startsWith('.') || path.isAbsolute(modulePath)) {
-    const packageRelativeModulePath = path.relative(
-      containingPackage.rootPath,
-      path.resolve(path.dirname(originModulePath), modulePath),
-    );
+  if (modulePath.startsWith('.') || isModulePathAbsolute) {
+    const packageRelativeModulePath = isModulePathAbsolute
+      ? // If the module path is absolute, containingPackage is relative to it
+        // (see above).
+        containingPackage.packageRelativePath
+      : // Otherwise containingPackage is relative to the origin module.
+        // Origin's package-relative directory joined with the target module's
+        // origin-relative path gives us the module's package-relative path.
+        path.join(
+          path.dirname(containingPackage.packageRelativePath),
+          modulePath,
+        );
+
     redirectedPath = matchSubpathFromMainFields(
       // Use prefixed POSIX path for lookup in package.json
       './' + toPosixPath(packageRelativeModulePath),

packages/metro-resolver/src/__tests__/package-exports-test.js

@@ -306,7 +306,7 @@ describe('with package exports resolution enabled', () => {
       });
       expect(logWarning).toHaveBeenCalledTimes(1);
       expect(logWarning.mock.calls[0][0]).toMatchInlineSnapshot(
-        `"Attempted to import the module \\"/root/node_modules/test-pkg/foo\\" which is not listed in the \\"exports\\" of \\"/root/node_modules/test-pkg\\". Falling back to file-based resolution. Consider updating the call site or asking the package maintainer(s) to expose this API."`,
+        `"Attempted to import the module \\"/root/node_modules/test-pkg/foo\\" which is not listed in the \\"exports\\" of \\"/root/node_modules/test-pkg\\" under the requested subpath \\"./foo\\". Falling back to file-based resolution. Consider updating the call site or asking the package maintainer(s) to expose this API."`,
       );
     });
 
@@ -454,7 +454,7 @@ describe('with package exports resolution enabled', () => {
         );
         expect(logWarning).toHaveBeenCalledTimes(1);
         expect(logWarning.mock.calls[0][0]).toMatchInlineSnapshot(
-          `"Attempted to import the module \\"/root/node_modules/test-pkg/private/bar\\" which is not listed in the \\"exports\\" of \\"/root/node_modules/test-pkg\\". Falling back to file-based resolution. Consider updating the call site or asking the package maintainer(s) to expose this API."`,
+          `"Attempted to import the module \\"/root/node_modules/test-pkg/private/bar\\" which is not listed in the \\"exports\\" of \\"/root/node_modules/test-pkg\\" under the requested subpath \\"./private/bar\\". Falling back to file-based resolution. Consider updating the call site or asking the package maintainer(s) to expose this API."`,
         );
       });
 

packages/metro-resolver/src/__tests__/utils.js

@@ -113,6 +113,7 @@ export function createPackageAccessors(
         return {
           rootPath: dir,
           packageJson,
+          packageRelativePath: path.relative(dir, modulePath),
         };
       }
 

packages/metro-resolver/src/resolve.js

@@ -279,6 +279,7 @@ function resolvePackage(
           {...context, unstable_conditionNames: conditionNamesOverride},
           pkg.rootPath,
           absoluteCandidatePath,
+          pkg.packageRelativePath,
           exportsField,
           platform,
         );

packages/metro-resolver/src/types.js

@@ -86,6 +86,13 @@ export type PackageInfo = $ReadOnly<{
   rootPath: string,
 }>;
 
+export type PackageForModule = $ReadOnly<{
+  ...PackageInfo,
+  /* A system-separated subpath (with no './' prefix) that reflects the subpath
+     of the given candidate relative to the returned rootPath. */
+  packageRelativePath: string,
+}>;
+
 /**
  * Check existence of a single file.
  */
@@ -121,13 +128,13 @@ export type ResolutionContext = $ReadOnly<{
   getPackage: (packageJsonPath: string) => ?PackageJson,
 
   /**
-   * Get the closest package scope and parsed `package.json` for a given
-   * absolute candidate path (which need not exist), or null if there is no
-   * package.json closer than the nearest node_modules directory.
+   * Get the closest package scope, parsed `package.json` and relative subpath
+   * for a given absolute candidate path (which need not exist), or null if
+   * there is no package.json closer than the nearest node_modules directory.
    *
    * @deprecated See https://github.com/facebook/metro/commit/29c77bff31e2475a086bc3f04073f485da8f9ff0
    */
-  getPackageForModule: (absoluteModulePath: string) => ?PackageInfo,
+  getPackageForModule: (absoluteModulePath: string) => ?PackageForModule,
 
   /**
    * The dependency descriptor, within the origin module, corresponding to the

packages/metro-resolver/types/types.d.ts

@@ -64,6 +64,12 @@ export interface PackageInfo {
   readonly rootPath: string;
 }
 
+export interface PackageForModule extends PackageInfo {
+  /* A system-separated subpath (with no './' prefix) that reflects the subpath
+     of the given candidate relative to the returned rootPath. */
+  readonly packageRelativePath: string;
+}
+
 /**
  * Check existence of a single file.
  */
@@ -97,15 +103,15 @@ export interface ResolutionContext {
   readonly getPackage: (packageJsonPath: string) => PackageJson | null;
 
   /**
-   * Get the closest package scope and parsed `package.json` for a given
-   * absolute candidate path (which need not exist), or null if there is no
-   * package.json closer than the nearest node_modules directory.
+   * Get the closest package scope, parsed `package.json` and relative subpath
+   * for a given absolute candidate path (which need not exist), or null if
+   * there is no package.json closer than the nearest node_modules directory.
    *
    * @deprecated See https://github.com/facebook/metro/commit/29c77bff31e2475a086bc3f04073f485da8f9ff0
    */
   readonly getPackageForModule: (
     absoluteModulePath: string,
-  ) => PackageInfo | null;
+  ) => PackageForModule | null;
 
   /**
    * The dependency descriptor, within the origin module, corresponding to the

packages/metro/src/node-haste/DependencyGraph.js

@@ -152,7 +152,9 @@ class DependencyGraph extends EventEmitter {
     return self;
   }
 
-  _getClosestPackage(absoluteModulePath: string): ?string {
+  _getClosestPackage(
+    absoluteModulePath: string,
+  ): ?{packageJsonPath: string, packageRelativePath: string} {
     const parsedPath = path.parse(absoluteModulePath);
     const root = parsedPath.root;
     let dir = path.join(parsedPath.dir, parsedPath.base);
@@ -165,7 +167,12 @@ class DependencyGraph extends EventEmitter {
       }
       const candidate = path.join(dir, 'package.json');
       if (this._fileSystem.exists(candidate)) {
-        return candidate;
+        return {
+          packageJsonPath: candidate,
+          // Note that by construction, dir is a prefix of absoluteModulePath,
+          // so this relative path has no indirections.
+          packageRelativePath: path.relative(dir, absoluteModulePath),
+        };
       }
       dir = path.dirname(dir);
     } while (dir !== '.' && dir !== root);

packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js

@@ -25,7 +25,7 @@ import type {
   Resolution,
   ResolveAsset,
 } from 'metro-resolver';
-import type {PackageInfo, PackageJson} from 'metro-resolver/src/types';
+import type {PackageForModule, PackageJson} from 'metro-resolver/src/types';
 
 const {codeFrameColumns} = require('@babel/code-frame');
 const fs = require('fs');
@@ -53,7 +53,10 @@ export type ModuleishCache<TPackage> = interface {
     platform?: string,
     supportsNativePlatform?: boolean,
   ): TPackage,
-  getPackageOf(absolutePath: string): ?TPackage,
+  getPackageOf(absolutePath: string): ?{
+    pkg: TPackage,
+    packageRelativePath: string,
+  },
 };
 
 type Options<TPackage> = $ReadOnly<{
@@ -95,7 +98,7 @@ class ModuleResolver<TPackage: Packageish> {
     this._projectRootFakeModule = {
       path: path.join(projectRoot, '_'),
       getPackage: () =>
-        moduleCache.getPackageOf(this._projectRootFakeModule.path),
+        moduleCache.getPackageOf(this._projectRootFakeModule.path)?.pkg,
       isHaste() {
         throw new Error('not implemented');
       },
@@ -249,20 +252,21 @@ class ModuleResolver<TPackage: Packageish> {
     return null;
   };
 
-  _getPackageForModule = (absolutePath: string): ?PackageInfo => {
-    let pkg;
+  _getPackageForModule = (absolutePath: string): ?PackageForModule => {
+    let result;
 
     try {
-      pkg = this._options.moduleCache.getPackageOf(absolutePath);
+      result = this._options.moduleCache.getPackageOf(absolutePath);
     } catch (e) {
       // Do nothing. The standard module cache does not trigger any error, but
       // the ModuleGraph one does, if the module does not exist.
     }
 
-    return pkg != null
+    return result != null
       ? {
-          rootPath: path.dirname(pkg.path),
-          packageJson: pkg.read(),
+          rootPath: path.dirname(result.pkg.path),
+          packageJson: result.pkg.read(),
+          packageRelativePath: result.packageRelativePath,
         }
       : null;
   };

packages/metro/src/node-haste/Module.js

@@ -32,7 +32,7 @@ class Module {
   }
 
   getPackage(): ?Package {
-    return this._moduleCache.getPackageForModule(this);
+    return this._moduleCache.getPackageForModule(this)?.pkg;
   }
 
   invalidate() {}

packages/metro/src/node-haste/ModuleCache.js

@@ -14,7 +14,10 @@
 const Module = require('./Module');
 const Package = require('./Package');
 
-type GetClosestPackageFn = (absoluteFilePath: string) => ?string;
+type GetClosestPackageFn = (absoluteFilePath: string) => ?{
+  packageJsonPath: string,
+  packageRelativePath: string,
+};
 
 class ModuleCache {
   _getClosestPackage: GetClosestPackageFn;
@@ -29,8 +32,11 @@ class ModuleCache {
     ...
   };
   // Cache for "closest package.json" queries by module path.
-  _packagePathByModulePath: {
-    [filePath: string]: string,
+  _packagePathAndSubpathByModulePath: {
+    [filePath: string]: ?{
+      packageJsonPath: string,
+      packageRelativePath: string,
+    },
     __proto__: null,
     ...
   };
@@ -45,7 +51,7 @@ class ModuleCache {
     this._getClosestPackage = options.getClosestPackage;
     this._moduleCache = Object.create(null);
     this._packageCache = Object.create(null);
-    this._packagePathByModulePath = Object.create(null);
+    this._packagePathAndSubpathByModulePath = Object.create(null);
     this._modulePathsByPackagePath = Object.create(null);
   }
 
@@ -65,29 +71,45 @@ class ModuleCache {
     return this._packageCache[filePath];
   }
 
-  getPackageForModule(module: Module): ?Package {
+  getPackageForModule(
+    module: Module,
+  ): ?{pkg: Package, packageRelativePath: string} {
     return this.getPackageOf(module.path);
   }
 
-  getPackageOf(absoluteModulePath: string): ?Package {
-    let packagePath: ?string =
-      this._packagePathByModulePath[absoluteModulePath];
-    if (packagePath && this._packageCache[packagePath]) {
-      return this._packageCache[packagePath];
+  getPackageOf(
+    absoluteModulePath: string,
+  ): ?{pkg: Package, packageRelativePath: string} {
+    let packagePathAndSubpath =
+      this._packagePathAndSubpathByModulePath[absoluteModulePath];
+    if (
+      packagePathAndSubpath &&
+      this._packageCache[packagePathAndSubpath.packageJsonPath]
+    ) {
+      return {
+        pkg: this._packageCache[packagePathAndSubpath.packageJsonPath],
+        packageRelativePath: packagePathAndSubpath.packageRelativePath,
+      };
     }
 
-    packagePath = this._getClosestPackage(absoluteModulePath);
-    if (!packagePath) {
+    packagePathAndSubpath = this._getClosestPackage(absoluteModulePath);
+    if (!packagePathAndSubpath) {
       return null;
     }
 
-    this._packagePathByModulePath[absoluteModulePath] = packagePath;
+    const packagePath = packagePathAndSubpath.packageJsonPath;
+
+    this._packagePathAndSubpathByModulePath[absoluteModulePath] =
+      packagePathAndSubpath;
     const modulePaths =
       this._modulePathsByPackagePath[packagePath] ?? new Set();
     modulePaths.add(absoluteModulePath);
     this._modulePathsByPackagePath[packagePath] = modulePaths;
 
-    return this.getPackage(packagePath);
+    return {
+      pkg: this.getPackage(packagePath),
+      packageRelativePath: packagePathAndSubpath.packageRelativePath,
+    };
   }
 
   invalidate(filePath: string) {
@@ -99,10 +121,12 @@ class ModuleCache {
       this._packageCache[filePath].invalidate();
       delete this._packageCache[filePath];
     }
-    if (this._packagePathByModulePath[filePath]) {
+    const packagePathAndSubpath =
+      this._packagePathAndSubpathByModulePath[filePath];
+    if (packagePathAndSubpath) {
       // filePath is a module inside a package.
-      const packagePath = this._packagePathByModulePath[filePath];
-      delete this._packagePathByModulePath[filePath];
+      const packagePath = packagePathAndSubpath.packageJsonPath;
+      delete this._packagePathAndSubpathByModulePath[filePath];
       // This change doesn't invalidate any cached "closest package.json"
       // queries for the package's other modules. Clean up only this module.
       const modulePaths = this._modulePathsByPackagePath[packagePath];
@@ -118,7 +142,7 @@ class ModuleCache {
       // package.json" queries for modules inside this package.
       const modulePaths = this._modulePathsByPackagePath[filePath];
       for (const modulePath of modulePaths) {
-        delete this._packagePathByModulePath[modulePath];
+        delete this._packagePathAndSubpathByModulePath[modulePath];
       }
       modulePaths.clear();
       delete this._modulePathsByPackagePath[filePath];