Tweaks (remove name, validate duplicated paths)

Author: arcanis

doc/api/errors.md

@@ -2496,27 +2496,25 @@ A given value is out of the accepted range.
 The `package.json` [`"imports"`][] field does not define the given internal
 package specifier mapping.
 
-<a id="ERR_PACKAGE_MAP_ACCESS_DENIED"></a>
+<a id="ERR_PACKAGE_MAP_EXTERNAL_FILE"></a>
 
-### `ERR_PACKAGE_MAP_ACCESS_DENIED`
+### `ERR_PACKAGE_MAP_EXTERNAL_FILE`
 
 <!-- YAML
 added: REPLACEME
 -->
 
-A package attempted to import another package that exists in the [package map][]
-but is not listed in its `dependencies` object.
+A module attempted to resolve a bare specifier using the [package map][], but
+the importing file is not located within any package defined in the map.
 
-```mjs
-// package-map.json declares "app" with dependencies: {"utils": "utils"}
-// but "app" tries to import "secret-lib" which exists in the map
-
-// In app/index.js
-import secret from 'secret-lib'; // Throws ERR_PACKAGE_MAP_ACCESS_DENIED
+```console
+$ node --experimental-package-map=./package-map.json /tmp/script.js
+Error [ERR_PACKAGE_MAP_EXTERNAL_FILE]: Cannot resolve "dep-a" from "/tmp/script.js": file is not within any package defined in /path/to/package-map.json
 ```
 
-To fix this error, add the required package to the importing package's
-`dependencies` object in the package map configuration file.
+To fix this error, ensure the importing file is inside one of the package
+directories listed in the package map, or add a new package entry whose `path`
+covers the importing file.
 
 <a id="ERR_PACKAGE_MAP_INVALID"></a>
 
@@ -2532,6 +2530,7 @@ The [package map][] configuration file is invalid. This can occur when:
 * The file contains invalid JSON.
 * The file is missing the required `packages` object.
 * A package entry is missing the required `path` field.
+* Two package entries have the same `path` value.
 
 ```console
 $ node --experimental-package-map=./missing.json app.js
@@ -2553,7 +2552,6 @@ key that is not defined in the `packages` object.
 {
   "packages": {
     "app": {
-      "name": "app",
       "path": "./app",
       "dependencies": {
         "foo": "nonexistent"

doc/api/packages.md

@@ -987,19 +987,16 @@ Each key in `packages` is a unique identifier for a package entry:
 {
   "packages": {
     "app": {
-      "name": "my-app",
       "path": "./packages/app",
       "dependencies": {
         "@myorg/utils": "utils",
         "@myorg/ui-lib": "ui-lib"
       }
     },
     "utils": {
-      "name": "@myorg/utils",
       "path": "./packages/utils"
     },
     "ui-lib": {
-      "name": "@myorg/ui-lib",
       "path": "./packages/ui-lib",
       "dependencies": {
         "@myorg/utils": "utils"
@@ -1012,9 +1009,8 @@ Each key in `packages` is a unique identifier for a package entry:
 Each package entry has the following fields:
 
 * `path` {string} **Required.** Relative path from the configuration file to
-  the package directory.
-* `name` {string} The package name used in import specifiers. If omitted, the
-  package cannot be imported by name but can still import its dependencies.
+  the package directory. Each path must be unique across all packages in the
+  map; duplicate paths will throw an [`ERR_PACKAGE_MAP_INVALID`][] error.
 * `dependencies` {Object} An object mapping bare specifiers to package keys.
   Each key is the import name used in source code, and each value is the
   corresponding package key in the `packages` object. Defaults to an empty
@@ -1026,14 +1022,12 @@ When a bare specifier is encountered:
 
 1. Node.js determines which package contains the importing file by checking
    if the file path is within any package's `path`.
-2. If the importing file is not within any mapped package, a
-   `MODULE_NOT_FOUND` error is thrown.
+2. If the importing file is not within any mapped package, an
+   [`ERR_PACKAGE_MAP_EXTERNAL_FILE`][] error is thrown.
 3. Node.js looks up the specifier's package name in the importing package's
    `dependencies` object to find the corresponding package key.
 4. If found, the specifier resolves to the target package's `path`.
-5. If the package exists in the map but is not in `dependencies`, an
-   [`ERR_PACKAGE_MAP_ACCESS_DENIED`][] error is thrown.
-6. If the package does not exist in the map at all, a
+5. If the specifier is not in `dependencies`, a
    `MODULE_NOT_FOUND` error is thrown.
 
 ### Subpath resolution
@@ -1060,25 +1054,21 @@ can map the same specifier to different targets:
 {
   "packages": {
     "app": {
-      "name": "app",
       "path": "./app",
       "dependencies": {
         "component": "component-v2"
       }
     },
     "legacy": {
-      "name": "legacy",
       "path": "./legacy",
       "dependencies": {
         "component": "component-v1"
       }
     },
     "component-v1": {
-      "name": "component",
       "path": "./vendor/component-1.0.0"
     },
     "component-v2": {
-      "name": "component",
       "path": "./vendor/component-2.0.0"
     }
   }
@@ -1352,7 +1342,8 @@ This field defines [subpath imports][] for the current package.
 [`--experimental-addon-modules`]: cli.md#--experimental-addon-modules
 [`--experimental-package-map`]: cli.md#--experimental-package-mappath
 [`--no-addons` flag]: cli.md#--no-addons
-[`ERR_PACKAGE_MAP_ACCESS_DENIED`]: errors.md#err_package_map_access_denied
+[`ERR_PACKAGE_MAP_EXTERNAL_FILE`]: errors.md#err_package_map_external_file
+[`ERR_PACKAGE_MAP_INVALID`]: errors.md#err_package_map_invalid
 [`ERR_PACKAGE_PATH_NOT_EXPORTED`]: errors.md#err_package_path_not_exported
 [`ERR_UNKNOWN_FILE_EXTENSION`]: errors.md#err_unknown_file_extension
 [`package.json`]: #nodejs-packagejson-field-definitions

lib/internal/errors.js

@@ -1661,8 +1661,8 @@ E('ERR_PACKAGE_IMPORT_NOT_DEFINED', (specifier, packagePath, base) => {
   return `Package import specifier "${specifier}" is not defined${packagePath ?
     ` in package ${packagePath}package.json` : ''} imported from ${base}`;
 }, TypeError);
-E('ERR_PACKAGE_MAP_ACCESS_DENIED', (specifier, fromKey, configPath) => {
-  return `Package "${specifier}" is not a declared dependency of "${fromKey}" in ${configPath}`;
+E('ERR_PACKAGE_MAP_EXTERNAL_FILE', (specifier, parentPath, configPath) => {
+  return `Cannot resolve "${specifier}" from "${parentPath}": file is not within any package defined in ${configPath}`;
 }, Error);
 E('ERR_PACKAGE_MAP_INVALID', (configPath, reason) => {
   return `Invalid package-map.json at "${configPath}": ${reason}`;

lib/internal/modules/package_map.js

@@ -4,7 +4,6 @@ const {
   JSONParse,
   ObjectEntries,
   SafeMap,
-  SafeSet,
   StringPrototypeIndexOf,
   StringPrototypeSlice,
 } = primordials;
@@ -19,7 +18,7 @@ const getPackageMapPath = getLazy(() =>
 const fs = require('fs');
 
 const {
-  ERR_PACKAGE_MAP_ACCESS_DENIED,
+  ERR_PACKAGE_MAP_EXTERNAL_FILE,
   ERR_PACKAGE_MAP_INVALID,
   ERR_PACKAGE_MAP_KEY_NOT_FOUND,
 } = require('internal/errors').codes;
@@ -33,7 +32,6 @@ let emittedWarning = false;
  * @typedef {object} PackageMapEntry
  * @property {string} path - Absolute path to package on disk
  * @property {Map<string, string>} dependencies - Map from bare specifier to package key
- * @property {string|undefined} name - Package name (undefined = nameless package)
  */
 
 class PackageMap {
@@ -43,14 +41,10 @@ class PackageMap {
   #basePath;
   /** @type {Map<string, PackageMapEntry>} */
   #packages;
-  /** @type {Map<string, Set<string>>} */
-  #nameToKeys;
-  /** @type {Map<string, Set<string>>} */
-  #pathToKeys;
+  /** @type {Map<string, string>} */
+  #pathToKey;
   /** @type {Map<string, string|null>} */
   #pathToKeyCache;
-  /** @type {Map<string, object>} */
-  #resolveCache;
 
   /**
    * @param {string} configPath
@@ -60,10 +54,8 @@ class PackageMap {
     this.#configPath = configPath;
     this.#basePath = dirname(configPath);
     this.#packages = new SafeMap();
-    this.#nameToKeys = new SafeMap();
-    this.#pathToKeys = new SafeMap();
+    this.#pathToKey = new SafeMap();
     this.#pathToKeyCache = new SafeMap();
-    this.#resolveCache = new SafeMap();
 
     this.#parse(data);
   }
@@ -86,22 +78,19 @@ class PackageMap {
       this.#packages.set(key, {
         path: absolutePath,
         dependencies: new SafeMap(ObjectEntries(entry.dependencies ?? {})),
-        name: entry.name,  // Undefined for nameless packages
       });
 
-      // Index by name (only named packages)
-      if (entry.name !== undefined) {
-        if (!this.#nameToKeys.has(entry.name)) {
-          this.#nameToKeys.set(entry.name, new SafeSet());
-        }
-        this.#nameToKeys.get(entry.name).add(key);
+      // TODO(arcanis): Duplicates should be tolerated, but it requires changing module IDs
+      // to include the package ID whenever a package map is used.
+      if (this.#pathToKey.has(absolutePath)) {
+        const existingKey = this.#pathToKey.get(absolutePath);
+        throw new ERR_PACKAGE_MAP_INVALID(
+          this.#configPath,
+          `packages "${existingKey}" and "${key}" have the same path "${entry.path}"; this will be supported in the future`,
+        );
       }
 
-      // Index by path
-      if (!this.#pathToKeys.has(absolutePath)) {
-        this.#pathToKeys.set(absolutePath, new SafeSet());
-      }
-      this.#pathToKeys.get(absolutePath).add(key);
+      this.#pathToKey.set(absolutePath, key);
     }
   }
 
@@ -117,9 +106,8 @@ class PackageMap {
     // Walk up to find containing package
     let checkPath = filePath;
     while (true) {
-      const keys = this.#pathToKeys.get(checkPath);
-      if (keys && keys.size > 0) {
-        const key = keys.values().next().value;
+      const key = this.#pathToKey.get(checkPath);
+      if (key !== undefined) {
         this.#pathToKeyCache.set(filePath, key);
         return key;
       }
@@ -135,49 +123,26 @@ class PackageMap {
   /**
    * Main resolution method.
    * Returns the package path and subpath for the specifier, or undefined if
-   * resolution should fall back to standard resolution.
+   * the specifier is not in the parent package's dependencies.
+   * Throws ERR_PACKAGE_MAP_EXTERNAL_FILE if parentPath is not within any
+   * mapped package.
    * @param {string} specifier
    * @param {string} parentPath - File path of the importing module
    * @returns {{packagePath: string, subpath: string}|undefined}
    */
   resolve(specifier, parentPath) {
     const parentKey = this.#getKeyForPath(parentPath);
 
-    // Parent not in map - let the caller throw the appropriate error
-    if (parentKey === null) { return undefined; }
-
-    // Check cache
-    const cacheKey = `${parentKey}\0${specifier}`;
-    if (this.#resolveCache.has(cacheKey)) {
-      return this.#resolveCache.get(cacheKey);
+    if (parentKey === null) {
+      throw new ERR_PACKAGE_MAP_EXTERNAL_FILE(specifier, parentPath, this.#configPath);
     }
 
-    const result = this.#resolveUncached(specifier, parentKey);
-    this.#resolveCache.set(cacheKey, result);
-    return result;
-  }
-
-  /**
-   * @param {string} specifier
-   * @param {string} parentKey
-   * @returns {{packagePath: string, subpath: string}|undefined}
-   */
-  #resolveUncached(specifier, parentKey) {
     const { packageName, subpath } = parsePackageName(specifier);
     const parentEntry = this.#packages.get(parentKey);
 
     const targetKey = parentEntry.dependencies.get(packageName);
-
     if (targetKey === undefined) {
-      // Check if package exists anywhere in map but isn't accessible
-      if (this.#nameToKeys.has(packageName)) {
-        throw new ERR_PACKAGE_MAP_ACCESS_DENIED(
-          specifier,
-          parentKey,
-          this.#configPath,
-        );
-      }
-      // Package not in map at all - let the caller throw the appropriate error
+      // Package not in parent's dependencies - let the caller throw the appropriate error
       return undefined;
     }
 

test/es-module/test-esm-package-map.mjs

@@ -49,39 +49,6 @@ describe('ESM: --experimental-package-map', () => {
     });
   });
 
-  // =========== Access Control ===========
-
-  describe('dependency access control', () => {
-    it('throws ERR_PACKAGE_MAP_ACCESS_DENIED for undeclared dependency', async () => {
-      const { code, stderr } = await spawnPromisified(execPath, [
-        '--experimental-package-map', packageMapPath,
-        '--input-type=module',
-        '--eval',
-        `import x from 'not-a-dep';`,
-      ], { cwd: fixtures.path('package-map/root') });
-
-      assert.notStrictEqual(code, 0);
-      assert.match(stderr, /ERR_PACKAGE_MAP_ACCESS_DENIED/);
-      assert.match(stderr, /not-a-dep/);
-      assert.match(stderr, /root/);  // parent package name
-    });
-
-    it('includes package key in error for nameless packages', async () => {
-      const { code, stderr } = await spawnPromisified(execPath, [
-        '--experimental-package-map',
-        fixtures.path('package-map/package-map-nameless.json'),
-        '--input-type=module',
-        '--eval',
-        `import x from 'forbidden';`,
-      ], { cwd: fixtures.path('package-map/nameless-pkg') });
-
-      assert.notStrictEqual(code, 0);
-      assert.match(stderr, /ERR_PACKAGE_MAP_ACCESS_DENIED/);
-      // Should show key since package is nameless
-      assert.match(stderr, /nameless/);
-    });
-  });
-
   // =========== Fallback Behavior ===========
 
   describe('resolution boundaries', () => {
@@ -106,7 +73,7 @@ describe('ESM: --experimental-package-map', () => {
       ], { cwd: '/tmp' });  // Not in any mapped package
 
       assert.notStrictEqual(code, 0);
-      assert.match(stderr, /ERR_MODULE_NOT_FOUND/);
+      assert.match(stderr, /ERR_PACKAGE_MAP_EXTERNAL_FILE/);
     });
   });
 
@@ -162,6 +129,20 @@ describe('ESM: --experimental-package-map', () => {
       assert.match(stderr, /ERR_PACKAGE_MAP_INVALID/);
       assert.match(stderr, /not found/);
     });
+
+    it('throws ERR_PACKAGE_MAP_INVALID for duplicate package paths', async () => {
+      const { code, stderr } = await spawnPromisified(execPath, [
+        '--experimental-package-map',
+        fixtures.path('package-map/package-map-duplicate-path.json'),
+        '--input-type=module',
+        '--eval', `import x from 'dep-a';`,
+      ], { cwd: fixtures.path('package-map/root') });
+
+      assert.notStrictEqual(code, 0);
+      assert.match(stderr, /ERR_PACKAGE_MAP_INVALID/);
+      assert.match(stderr, /pkg-a/);
+      assert.match(stderr, /pkg-b/);
+    });
   });
 
   // =========== Zero Overhead When Disabled ===========
@@ -302,9 +283,9 @@ describe('ESM: --experimental-package-map', () => {
       assert.match(stdout, /inner using dep-a-value/);
     });
 
-    it('denies access to nested package deps from parent package', async () => {
+    it('falls back for undeclared dependency in nested package parent', async () => {
       // Root does not list dep-a in its dependencies, so importing it
-      // from root should fail even though inner (nested inside root) can.
+      // from root should fail with ERR_MODULE_NOT_FOUND.
       const { code, stderr } = await spawnPromisified(execPath, [
         '--experimental-package-map', longestPathMap,
         '--input-type=module',
@@ -313,7 +294,7 @@ describe('ESM: --experimental-package-map', () => {
       ], { cwd: fixtures.path('package-map/root') });
 
       assert.notStrictEqual(code, 0);
-      assert.match(stderr, /ERR_PACKAGE_MAP_ACCESS_DENIED/);
+      assert.match(stderr, /ERR_MODULE_NOT_FOUND/);
     });
   });
 });