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;
   };