Apply suggestions from code review

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Author: arcanis

doc/api/packages.md

@@ -953,7 +953,7 @@ See [the package examples repository][] for details.
 added: REPLACEME
 -->
 
-> Stability: 1 - Experimental
+> Stability: 1 - Experimental. Enable this API with [`--experimental-package-map`][].
 
 Package maps provide a mechanism to control package resolution without relying
 on the `node_modules` folder structure. When enabled via the
@@ -969,15 +969,6 @@ This feature is useful for:
 * **Multiple versions**: Allow different packages to depend on different
   versions of the same dependency.
 
-### Enabling package maps
-
-Package maps are enabled by passing the `--experimental-package-map` flag
-with a path to the configuration file:
-
-```bash
-node --experimental-package-map=./package-map.json app.js
-```
-
 ### Configuration file format
 
 The package map configuration file is a JSON file with a `packages` object.
@@ -1107,7 +1098,7 @@ originates from:
 
 In the example above both `lib-old` and `lib-new` use the same `./lib` folder to
 store their sources, the only difference being in which version of `react` they'll
-access when performing require calls.
+access when performing `require` calls or using `import`.
 
 Because multiple package entries share the same path, resolving a bare specifier
 from a file within that path is ambiguous unless the originating package ID is
@@ -1120,21 +1111,6 @@ and propagate it from each resolution result to subsequent resolution requests.
 This ensures that when `lib` requires `react`, the runtime knows whether the
 request comes from `lib-old` or `lib-new` and can select the correct dependency.
 
-### CommonJS and ES modules
-
-Package maps work with both CommonJS (`require()`) and ES modules (`import`).
-The resolution behavior is identical for both module systems.
-
-```cjs
-// CommonJS
-const utils = require('@myorg/utils');
-```
-
-```mjs
-// ES modules
-import utils from '@myorg/utils';
-```
-
 ### Interaction with other resolution
 
 Package maps only apply to bare specifiers that are not Node.js builtin

lib/internal/modules/cjs/loader.js

@@ -670,8 +670,8 @@ function tryPackageMapResolveCJS(request, parent, conditions) {
     const message = getRequireStackMessage(request, requireStack);
     // eslint-disable-next-line no-restricted-syntax
     const err = new Error(message);
-    err.code = 'MODULE_NOT_FOUND';
-    err.requireStack = requireStack;
+    setOwnProperty(err, 'code', 'MODULE_NOT_FOUND');
+    setOwnProperty(err, 'requireStack', requireStack);
     throw err;
   }
 
@@ -1518,8 +1518,8 @@ Module._resolveFilename = function(request, parent, isMain, options) {
 
   // eslint-disable-next-line no-restricted-syntax
   const err = new Error(message);
-  err.code = 'MODULE_NOT_FOUND';
-  err.requireStack = requireStack;
+  setOwnProperty(err, 'code', 'MODULE_NOT_FOUND');
+  setOwnProperty(err, 'requireStack', requireStack);
 
   throw err;
 };

lib/internal/modules/package_map.js

@@ -71,7 +71,9 @@ class PackageMap {
       throw new ERR_PACKAGE_MAP_INVALID(this.#configPath, 'missing "packages" object');
     }
 
-    for (const { 0: key, 1: entry } of ObjectEntries(data.packages)) {
+    const entries = ObjectEntries(data.packages);
+    for (let i = 0; i < entries.length; i++) {
+      const { 0: key, 1: entry } = entry;
       if (!entry.path) {
         throw new ERR_PACKAGE_MAP_INVALID(this.#configPath, `package "${key}" is missing "path" field`);
       }
@@ -99,8 +101,8 @@ class PackageMap {
 
       // 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);
+      const existingKey = this.#pathToKey.get(absolutePath);
+      if (existingKey != null) {
         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`,

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

@@ -39,8 +39,9 @@ describe('ESM: --experimental-package-map', () => {
         `import dep from 'dep-a'; console.log(dep);`,
       ], { cwd: fixtures.path('package-map/root') });
 
-      assert.strictEqual(code, 0, stderr);
+      assert.strictEqual(stderr, '');
       assert.match(stdout, /dep-a-value/);
+      assert.strictEqual(code, 0);
     });
 
     it('resolves a subpath export', async () => {

test/parallel/test-package-map-cli.js

@@ -39,9 +39,9 @@ describe('--experimental-package-map CLI behavior', () => {
       encoding: 'utf8',
     });
 
-    assert.strictEqual(status, 0);
     assert.match(stderr, /ExperimentalWarning/);
     assert.match(stderr, /[Pp]ackage map/);
+    assert.strictEqual(status, 0);
   });
 
   it('accepts relative path', () => {

test/parallel/test-require-package-map.js

@@ -28,7 +28,7 @@ writeFileSync(fileUrlFixturePath, JSON.stringify({
   },
 }));
 
-describe('CJS: --experimental-package-map', () => {
+describe('CJS: --experimental-package-map', { concurrency: !process.env.TEST_PARALLEL }, () => {
 
   describe('basic resolution', () => {
     it('resolves require() through package map', () => {