Implement file extension resolution for CJS, same as ESM; add exhaustive tests for resolver

.github/workflows/continuous-integration.yml

@@ -54,7 +54,6 @@ jobs:
         flavor: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]
         include:
           # Node 12.15
-          # TODO Add comments about why we test 12.15; I think git blame says it's because of an ESM behavioral change that happened at 12.16
           - flavor: 1
             node: 12.15
             nodeFlag: 12_15
@@ -122,10 +121,10 @@ jobs:
             typescript: next
             typescriptFlag: next
             downgradeNpm: true
-          # Node 17
+          # Node 18
           - flavor: 12
-            node: 17
-            nodeFlag: 17
+            node: 18
+            nodeFlag: 18
             typescript: latest
             typescriptFlag: latest
             downgradeNpm: true

.gitignore

@@ -1,14 +1,15 @@
 /node_modules/
 /tests/node_modules/
-.nyc_output/
-coverage/
+/.nyc_output/
+/coverage/
 .DS_Store
 npm-debug.log
 /dist/
-tsconfig.schema.json
-tsconfig.schemastore-schema.json
-.idea/
-/.vscode/
+/tsconfig.schema.json
+/tsconfig.schemastore-schema.json
+/.idea/
+/.vscode/*
+!/.vscode/launch.json
 /website/static/api
 /tsconfig.tsbuildinfo
 /temp

.vscode/launch.json

@@ -12,6 +12,16 @@
         "<node_internals>/**/*.js"
       ],
     },
+    {
+      "name": "Debug resolver tests (example)",
+      "type": "pwa-node",
+      "request": "launch",
+      "cwd": "${workspaceFolder}/tests/tmp/resolver-0015-preferSrc-typeModule-allowJs-experimentalSpecifierResolutionNode",
+      "runtimeArgs": [
+        "--loader", "../../../esm.mjs"
+      ],
+      "program": "./src/entrypoint-0054-src-to-src.mjs"
+    },
     {
       "name": "Debug Example: running a test fixture against local ts-node/esm loader",
       "type": "pwa-node",
@@ -24,5 +34,5 @@
         "<node_internals>/**/*.js"
       ],
     }
-  ],
+  ]
 }

NOTES.md

@@ -0,0 +1,61 @@
+*Delete this file before merging this PR*
+
+## PnP interop
+
+Asked about it here:
+https://discord.com/channels/226791405589233664/654372321225605128/957301175609344070
+
+PnP API checks if import specifiers are for dependencies: non-relative, non-absolute
+  libfoo
+  @scope/libfoo
+
+When they are, it does `resolveToUnqualified` to map to an unqualified path.
+This path points to the module's location on disk (in a zip, perhaps) but does
+not handle file extension resolution or stuff like that.
+
+To interop with PnP, we need PnP to *only* `resolveToUnqualified`.
+We do everything else.
+
+```typescript
+import { Module } from 'module';
+import fs from 'fs';
+
+const pathRegExp = /^(?![a-zA-Z]:[\\/]|\\\\|\.{0,2}(?:\/|$))((?:@[^/]+\/)?[^/]+)\/*(.*|)$/;
+
+const originalModuleResolveFilename = Module._resolveFilename;
+Module._resolveFilename = function (
+    request: string,
+    parent: typeof Module | null | undefined,
+    isMain: boolean,
+    options?: { [key: string]: any }
+) {
+    const dependencyNameMatch = request.match(pathRegExp);
+    if (dependencyNameMatch !== null) {
+
+      const [, dependencyName, subPath] = dependencyNameMatch;
+
+      const unqualified = pnpapi.resolveToUnqualified(....);
+
+      // Do your modified resolution on the unqualified path here
+
+    } else {
+
+      // Do your modified resolution here; no need for PnP
+
+    }
+
+};
+```
+
+PnP can be installed at runtime.
+
+To conditionally check if PnP is available at the start of *every* resolution:
+
+```typescript
+// Get the pnpapi of either the issuer or the specifier.
+// The latter is required when the specifier is an absolute path to a
+// zip file and the issuer doesn't belong to a pnpapi
+const {findPnPApi} = Module;
+const pnpapi = findPnPApi ? (findPnpApi(issuer) ?? (url ? findPnpApi(specifier) : null)) : null;
+if (pnpapi) {...}
+```

TODO.md

@@ -0,0 +1,14 @@
+*Delete this file before merging this PR*
+
+## TODOs
+
+Copy any relevant changes from `add-cjs-loader-resolve`
+
+I forgot exactly where I was in `add-cjs-loader-resolve`
+Re-do the renaming and moving that I did in that branch.
+Then diff to see that I did it correctly.
+Avoid introducing any accidental behavioral changes.
+
+Make list of changes from vanilla node in dist-raw/node-internal-modules-cjs-loader-old.js
+Apply those changes to                    dist-raw/node-internal-modules-cjs-loader.js
+

ava.config.cjs

@@ -1,4 +1,5 @@
 const expect = require('expect');
+const semver = require('semver');
 const { createRequire } = require('module');
 
 module.exports = {
@@ -14,6 +15,9 @@ module.exports = {
     NODE_PATH: ''
   },
   require: ['./src/test/remove-env-var-force-color.js'],
+  nodeArguments: semver.gte(process.version, '14.0.0')
+    ? ['--loader', './src/test/test-loader.mjs', '--no-warnings']
+    : [],
   timeout: '300s',
   concurrency: 1,
 };

child-loader.mjs

@@ -2,6 +2,7 @@ import { fileURLToPath } from 'url';
 import { createRequire } from 'module';
 const require = createRequire(fileURLToPath(import.meta.url));
 
+// TODO why use require() here?  I think we can just `import`
 /** @type {import('./dist/child-loader')} */
 const childLoader = require('./dist/child/child-loader');
 export const { resolve, load, getFormat, transformSource } = childLoader;

dist-raw/node-internal-constants.js

@@ -0,0 +1,4 @@
+// Copied from https://github.com/nodejs/node/blob/master/lib/internal/constants.js
+module.exports = {
+  CHAR_FORWARD_SLASH: 47, /* / */
+};

dist-raw/node-internal-errors.js

@@ -2,9 +2,37 @@
 
 const path = require('path');
 
-module.exports = {
-  createErrRequireEsm
-};
+exports.codes = {
+  ERR_INPUT_TYPE_NOT_ALLOWED: createErrorCtor(joinArgs('ERR_INPUT_TYPE_NOT_ALLOWED')),
+  ERR_INVALID_ARG_VALUE: createErrorCtor(joinArgs('ERR_INVALID_ARG_VALUE')),
+  ERR_INVALID_MODULE_SPECIFIER: createErrorCtor(joinArgs('ERR_INVALID_MODULE_SPECIFIER')),
+  ERR_INVALID_PACKAGE_CONFIG: createErrorCtor(joinArgs('ERR_INVALID_PACKAGE_CONFIG')),
+  ERR_INVALID_PACKAGE_TARGET: createErrorCtor(joinArgs('ERR_INVALID_PACKAGE_TARGET')),
+  ERR_MANIFEST_DEPENDENCY_MISSING: createErrorCtor(joinArgs('ERR_MANIFEST_DEPENDENCY_MISSING')),
+  ERR_MODULE_NOT_FOUND: createErrorCtor((path, base, type = 'package') => {
+    return `Cannot find ${type} '${path}' imported from ${base}`
+  }),
+  ERR_PACKAGE_IMPORT_NOT_DEFINED: createErrorCtor(joinArgs('ERR_PACKAGE_IMPORT_NOT_DEFINED')),
+  ERR_PACKAGE_PATH_NOT_EXPORTED: createErrorCtor(joinArgs('ERR_PACKAGE_PATH_NOT_EXPORTED')),
+  ERR_UNSUPPORTED_DIR_IMPORT: createErrorCtor(joinArgs('ERR_UNSUPPORTED_DIR_IMPORT')),
+  ERR_UNSUPPORTED_ESM_URL_SCHEME: createErrorCtor(joinArgs('ERR_UNSUPPORTED_ESM_URL_SCHEME')),
+  ERR_UNKNOWN_FILE_EXTENSION: createErrorCtor(joinArgs('ERR_UNKNOWN_FILE_EXTENSION')),
+}
+
+function joinArgs(name) {
+  return (...args) => {
+    return [name, ...args].join(' ')
+  }
+}
+
+function createErrorCtor(errorMessageCreator) {
+  return class CustomError extends Error {
+    constructor(...args) {
+      super(errorMessageCreator(...args))
+    }
+  }
+}
+exports.createErrRequireEsm = createErrRequireEsm;
 
 // Native ERR_REQUIRE_ESM Error is declared here:
 //   https://github.com/nodejs/node/blob/2d5d77306f6dff9110c1f77fefab25f973415770/lib/internal/errors.js#L1294-L1313

dist-raw/node-internal-modules-cjs-helpers.js

@@ -1,11 +1,45 @@
-const {ArrayPrototypeForEach, StringPrototypeStartsWith, ObjectPrototypeHasOwnProperty, StringPrototypeIncludes, ObjectDefineProperty} = require('./node-primordials');
+// Copied from https://github.com/nodejs/node/blob/v17.0.1/lib/internal/modules/cjs/helpers.js
 
-exports.addBuiltinLibsToObject = addBuiltinLibsToObject;
+'use strict';
+
+const {
+  ArrayPrototypeForEach,
+  ObjectDefineProperty,
+  ObjectPrototypeHasOwnProperty,
+  SafeSet,
+  StringPrototypeIncludes,
+  StringPrototypeStartsWith,
+} = require('./node-primordials');
+
+const { getOptionValue } = require('./node-options');
+const userConditions = getOptionValue('--conditions');
+
+const noAddons = getOptionValue('--no-addons');
+const addonConditions = noAddons ? [] : ['node-addons'];
+
+// TODO: Use this set when resolving pkg#exports conditions in loader.js.
+const cjsConditions = new SafeSet([
+  'require',
+  'node',
+  ...addonConditions,
+  ...userConditions,
+]);
 
-// Copied from https://github.com/nodejs/node/blob/21f5a56914a3b24ad77535ef369b93c6b1c11d18/lib/internal/modules/cjs/helpers.js#L133-L178
-function addBuiltinLibsToObject(object) {
+/**
+ * @param {any} object
+ * @param {string} [dummyModuleName]
+ * @return {void}
+ */
+function addBuiltinLibsToObject(object, dummyModuleName) {
   // Make built-in modules available directly (loaded lazily).
-  const { builtinModules } = require('module').Module;
+  const Module = require('module').Module;
+  const { builtinModules } = Module;
+
+  // To require built-in modules in user-land and ignore modules whose
+  // `canBeRequiredByUsers` is false. So we create a dummy module object and not
+  // use `require()` directly.
+  const dummyModule = new Module(dummyModuleName);
+
   ArrayPrototypeForEach(builtinModules, (name) => {
     // Neither add underscored modules, nor ones that contain slashes (e.g.,
     // 'fs/promises') or ones that are already defined.
@@ -29,7 +63,8 @@ function addBuiltinLibsToObject(object) {
 
     ObjectDefineProperty(object, name, {
       get: () => {
-        const lib = require(name);
+        // Node 12 hack; remove when we drop node12 support
+        const lib = (dummyModule.require || require)(name);
 
         // Disable the current getter/setter and set up a new
         // non-enumerable property.
@@ -49,3 +84,6 @@ function addBuiltinLibsToObject(object) {
     });
   });
 }
+
+exports.addBuiltinLibsToObject = addBuiltinLibsToObject;
+exports.cjsConditions = cjsConditions;