improve cjs scanner

esinstall/src/index.ts

@@ -53,27 +53,6 @@ type DependencyLoc = {
   loc: string;
 };
 
-// Add popular CJS packages here that use "synthetic" named imports in their documentation.
-// CJS packages should really only be imported via the default export:
-//   import React from 'react';
-// But, some large projects use named exports in their documentation:
-//   import {useState} from 'react';
-//
-// We use "/index.js here to match the official package, but not any ESM aliase packages
-// that the user may have installed instead (ex: react-esm).
-const CJS_PACKAGES_TO_AUTO_DETECT = [
-  'react/index.js',
-  'react-dom/index.js',
-  'react-dom/server.js',
-  'react-is/index.js',
-  'prop-types/index.js',
-  'scheduler/index.js',
-  'react-table',
-  'chai/index.js',
-  'events/events.js',
-  'uuid/index.js',
-];
-
 function isImportOfPackage(importUrl: string, packageName: string) {
   return packageName === importUrl || importUrl.startsWith(packageName + '/');
 }
@@ -136,6 +115,7 @@ interface InstallOptions {
   externalEsm: string[] | ((imp: string) => boolean);
   packageLookupFields: string[];
   packageExportLookupFields: string[];
+  // @deprecated No longer needed, all packages now have the highest fidelity named export support possible
   namedExports: string[];
   rollup: {
     context?: string;
@@ -202,7 +182,6 @@ export async function install(
     importMap: _importMap,
     logger,
     dest: destLoc,
-    namedExports,
     external,
     externalEsm,
     sourcemap,
@@ -237,7 +216,6 @@ export async function install(
   const importMap: ImportMap = {imports: {}};
   let dependencyStats: DependencyStatsOutput | null = null;
   const skipFailures = false;
-  const autoDetectNamedExports = [...CJS_PACKAGES_TO_AUTO_DETECT, ...namedExports];
 
   for (const installSpecifier of allInstallSpecifiers) {
     let targetName = getWebDependencyName(installSpecifier);
@@ -360,7 +338,7 @@ ${colors.dim(
             : (externalEsm as Function)(id),
         requireReturnsDefault: 'auto',
       } as RollupCommonJSOptions),
-      rollupPluginWrapInstallTargets(!!isTreeshake, autoDetectNamedExports, installTargets, logger),
+      rollupPluginWrapInstallTargets(!!isTreeshake, installTargets, logger),
       stats && rollupPluginDependencyStats((info) => (dependencyStats = info)),
       rollupPluginNodeProcessPolyfill(env),
       polyfillNode && rollupPluginNodePolyfills(),
@@ -417,11 +395,9 @@ ${colors.dim(
   if (Object.keys(installEntrypoints).length > 0) {
     try {
       logger.debug(process.cwd());
-      logger.debug(`running installer with options: ${util.format(inputOptions)}`);
+      logger.debug(`running installer with options: ${JSON.stringify(inputOptions)}`);
       const packageBundle = await rollup(inputOptions);
-      logger.debug(
-        `installing npm packages:\n    ${Object.keys(installEntrypoints).join('\n    ')}`,
-      );
+      logger.debug(`installing npm packages: ${Object.keys(installEntrypoints).join(', ')}`);
       if (isFatalWarningFound) {
         throw new Error(FAILED_INSTALL_MESSAGE);
       }

esinstall/src/rollup-plugins/rollup-plugin-wrap-install-targets.ts

@@ -1,16 +1,26 @@
 import fs from 'fs';
-import * as colors from 'kleur/colors';
 import path from 'path';
 import {Plugin} from 'rollup';
-import {VM as VM2} from 'vm2';
+import execa from 'execa';
 import {AbstractLogger, InstallTarget} from '../types';
-import {getWebDependencyName, isJavaScript, isRemoteUrl, isTruthy, NATIVE_REQUIRE} from '../util';
+import {getWebDependencyName, isJavaScript, isRemoteUrl, isTruthy} from '../util';
 import isValidIdentifier from 'is-valid-identifier';
 import resolve from 'resolve';
 
 // Use CJS intentionally here! ESM interface is async but CJS is sync, and this file is sync
 const {parse} = require('cjs-module-lexer');
 
+function isValidNamedExport(name: string): boolean {
+  return name !== 'default' && name !== '__esModule' && isValidIdentifier(name);
+}
+
+// These packages are written so strangely, that our CJS scanner succeeds at scanning the file
+// but fails to pick up some export. Add popular packages here to save everyone a bit of
+// headache.
+// We use "/index.js here to match the official package, but not any ESM aliase packages
+// that the user may have installed instead (ex: react-esm).
+const UNSCANNABLE_CJS_PACKAGES = ['chai/index.js'];
+
 /**
  * rollup-plugin-wrap-install-targets
  *
@@ -25,40 +35,19 @@ const {parse} = require('cjs-module-lexer');
  */
 export function rollupPluginWrapInstallTargets(
   isTreeshake: boolean,
-  autoDetectPackageExports: string[],
   installTargets: InstallTarget[],
   logger: AbstractLogger,
 ): Plugin {
   const installTargetSummaries: {[loc: string]: InstallTarget} = {};
   const cjsScannedNamedExports = new Map<string, string[]>();
-  /**
-   * Runtime analysis: High Fidelity, but not always successful.
-   * `require()` the CJS file inside of Node.js to load the package and detect it's runtime exports.
-   * TODO: Safe to remove now that cjsAutoDetectExportsUntrusted() is getting smarter?
-   */
-  function cjsAutoDetectExportsTrusted(normalizedFileLoc: string): string[] | undefined {
-    try {
-      const mod = NATIVE_REQUIRE(normalizedFileLoc);
-      // Collect and filter all properties of the object as named exports.
-      return Object.keys(mod).filter((imp) => imp !== 'default' && imp !== '__esModule');
-    } catch (err) {
-      logger.debug(
-        `✘ Runtime CJS auto-detection for ${colors.bold(
-          normalizedFileLoc,
-        )} unsuccessful. Falling back to static analysis. ${err.message}`,
-      );
-    }
-  }
 
   /**
-   * Attempt #2: Static analysis: Lower Fidelity, but safe to run on anything.
-   * Get the exports that we scanned originally using static analysis. This is meant to run on
-   * any file (not only CJS) but it will only return an array if CJS exports were found.
+   * Attempt #1: Static analysis: Lower Fidelity, but faster.
+   * Do our best job to statically scan a file for named exports. This uses "cjs-module-lexer", the
+   * same CJS export scanner that Node.js uses internally. Very fast, but only works on some modules,
+   * depending on how they were build/written/compiled.
    */
-  function cjsAutoDetectExportsUntrusted(
-    filename: string,
-    visited = new Set(),
-  ): string[] | undefined {
+  function cjsAutoDetectExportsStatic(filename: string, visited = new Set()): string[] | undefined {
     const isMainEntrypoint = visited.size === 0;
     // Prevent infinite loops via circular dependencies.
     if (visited.has(filename)) {
@@ -77,40 +66,46 @@ export function rollupPluginWrapInstallTargets(
           [],
           reexports
             .map((e) =>
-              cjsAutoDetectExportsUntrusted(
+              cjsAutoDetectExportsStatic(
                 resolve.sync(e, {basedir: path.dirname(filename)}),
                 visited,
               ),
             )
             .filter(isTruthy),
         );
       }
-      // Attempt 2 - UMD: Run the file in a sandbox to dynamically analyze exports.
-      // This will only work on UMD and very simple CJS files (require not supported).
-      // Uses VM2 to run safely sandbox untrusted code (no access no Node.js primitives, just JS).
+      // If nothing was detected, return undefined.
       if (isMainEntrypoint && exports.length === 0 && reexports.length === 0) {
-        const vm = new VM2({wasm: false, fixAsync: false});
-        exports = Object.keys(
-          vm.run(
-            'const exports={}; const module={exports}; ' + fileContents + ';; module.exports;',
-          ),
-        );
-
-        // Verify that all of these are valid identifiers. Otherwise when we attempt to
-        // reexport it will produce invalid js like `import { a, b, 0, ) } from 'foo';
-        const allValid = exports.every((identifier) => isValidIdentifier(identifier));
-        if (!allValid) {
-          exports = [];
-        }
+        return undefined;
       }
+      // Otherwise, resolve and flatten all exports into a single array, remove invalid exports.
+      return Array.from(new Set([...exports, ...resolvedReexports])).filter(isValidNamedExport);
+    } catch (err) {
+      // Safe to ignore, this is usually due to the file not being CJS.
+      logger.debug(`cjsAutoDetectExportsStatic ${filename}: ${err.message}`);
+    }
+  }
 
-      // Resolve and flatten all exports into a single array, and remove invalid exports.
-      return Array.from(new Set([...exports, ...resolvedReexports])).filter(
-        (imp) => imp !== 'default' && imp !== '__esModule',
+  /**
+   * Attempt #2 - Runtime analysis: More powerful, but slower.
+   * This function spins off a Node.js process to analyze the most accurate set of named imports that this
+   * module supports. If this fails, there's not much else possible that we could do.
+   */
+  function cjsAutoDetectExportsRuntime(filename: string): string[] | undefined {
+    try {
+      const {stdout} = execa.sync(
+        `node`,
+        ['-p', `JSON.stringify(Object.keys(require('${filename}')))`],
+        {
+          cwd: __dirname,
+          extendEnv: false,
+        },
       );
+      const exportsResult = JSON.parse(stdout).filter(isValidNamedExport);
+      logger.debug(`cjsAutoDetectExportsRuntime success ${filename}: ${exportsResult}`);
+      return exportsResult;
     } catch (err) {
-      // Safe to ignore, this is usually due to the file not being CJS.
-      logger.debug(`cjsAutoDetectExportsUntrusted error: ${err.message}`);
+      logger.debug(`cjsAutoDetectExportsRuntime error ${filename}: ${err.message}`);
     }
   }
 
@@ -138,12 +133,14 @@ export function rollupPluginWrapInstallTargets(
         }, {} as any);
         installTargetSummaries[val] = installTargetSummary;
         const normalizedFileLoc = val.split(path.win32.sep).join(path.posix.sep);
-        const isExplicitAutoDetect = autoDetectPackageExports.some((p) =>
+        const knownBadPackage = UNSCANNABLE_CJS_PACKAGES.some((p) =>
           normalizedFileLoc.includes(`node_modules/${p}${p.endsWith('.js') ? '' : '/'}`),
         );
-        const cjsExports = isExplicitAutoDetect
-          ? cjsAutoDetectExportsTrusted(val)
-          : cjsAutoDetectExportsUntrusted(val);
+        const cjsExports =
+          // If we can trust the static analyzer, run that first.
+          (!knownBadPackage && cjsAutoDetectExportsStatic(val)) ||
+          // Otherwise, run the more powerful runtime analyzer.
+          cjsAutoDetectExportsRuntime(val);
         if (cjsExports && cjsExports.length > 0) {
           cjsScannedNamedExports.set(normalizedFileLoc, cjsExports);
           input[key] = `snowpack-wrap:${val}`;

snowpack/src/sources/local.ts

@@ -51,6 +51,10 @@ const NEVER_PEER_PACKAGES: string[] = [
   'whatwg-fetch',
 ];
 
+function isPackageEsm(packageManifest: any): boolean {
+  return !!(packageManifest.module || packageManifest.exports || packageManifest.type === 'module');
+}
+
 function getRootPackageDirectory(loc: string) {
   const parts = loc.split('node_modules');
   if (parts.length === 1) {
@@ -312,16 +316,14 @@ export default {
         // Look up the import map of the already-installed package.
         // If spec already exists, then this import map is valid.
         const lineBullet = colors.dim(depth === 0 ? '+' : '└──'.padStart(depth * 2 + 1, ' '));
-        const packageFormatted = spec + colors.dim('@' + packageVersion);
+        let packageFormatted = spec + colors.dim('@' + packageVersion);
         const existingImportMapLoc = path.join(installDest, 'import-map.json');
         const existingImportMap =
           (await fs.stat(existingImportMapLoc).catch(() => null)) &&
           JSON.parse(await fs.readFile(existingImportMapLoc, 'utf8'));
         if (existingImportMap && existingImportMap.imports[spec]) {
           if (depth > 0) {
             logger.info(`${lineBullet} ${packageFormatted} ${colors.dim(`(dedupe)`)}`);
-          } else {
-            logger.debug(`${lineBullet} ${packageFormatted} ${colors.dim(`(dedupe)`)}`);
           }
           return existingImportMap;
         }
@@ -366,7 +368,7 @@ export default {
               _packageName += '/' + specParts.shift();
             }
             const [, result] = resolveDependencyManifest(_packageName);
-            return !result || !!(result.module || result.exports || result.type === 'module');
+            return !result || isPackageEsm(result);
           },
         };
         if (config.packageOptions.source === 'local') {

test/esinstall/cjs-autodetect-exports/cjs-autodetect-exports.test.js

@@ -4,7 +4,7 @@ const {runTest} = require('../esinstall-test-utils.js');
 
 // This test simulates what keyboard-key is doing.
 describe('Auto-detecting CJS exports', () => {
-  it('should not attempt to convert package with invalid identifiers as exports', async () => {
+  it('should not convert invalid identifiers as exports', async () => {
     const cwd = __dirname;
     const dest = path.join(cwd, 'test-cjs-invalid-exports');
     const spec = 'cjs-invalid-exports';
@@ -18,8 +18,8 @@ describe('Auto-detecting CJS exports', () => {
 
     const output = fs.readFileSync(path.join(dest, `${spec}.js`), 'utf8');
     expect(output).toEqual(
-      // This shouldn't contain named exports
-      expect.not.stringContaining(`export {`),
+      // This shouldn't contain the ")" export
+      expect.stringContaining(`export { entrypoint as __moduleExports, a }`),
     );
   });