Prevent auto-exporting CJS modules with invalid identifiers (#2035)

This fixes keyboard-key

esinstall/package.json

@@ -48,6 +48,7 @@
     "cjs-module-lexer": "^1.0.0",
     "es-module-lexer": "^0.3.24",
     "is-builtin-module": "^3.0.0",
+    "is-valid-identifier": "^2.0.2",
     "kleur": "^4.1.1",
     "mkdirp": "^1.0.3",
     "rimraf": "^3.0.0",

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

@@ -5,6 +5,7 @@ import {Plugin} from 'rollup';
 import {VM as VM2} from 'vm2';
 import {AbstractLogger, InstallTarget} from '../types';
 import {getWebDependencyName, isJavaScript, isRemoteUrl, isTruthy} from '../util';
+import isValidIdentifier from 'is-valid-identifier';
 
 // Use CJS intentionally here! ESM interface is async but CJS is sync, and this file is sync
 const {parse} = require('cjs-module-lexer');
@@ -98,6 +99,13 @@ export function rollupPluginWrapInstallTargets(
             '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 = [];
+        }
       }
 
       // Resolve and flatten all exports into a single array, and remove invalid exports.

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

@@ -0,0 +1 @@
+test-*

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

@@ -0,0 +1,44 @@
+const fs = require('fs');
+const path = require('path');
+const {install} = require('../../../esinstall/lib');
+
+// 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 () => {
+    const cwd = __dirname;
+    const dest = path.join(cwd, 'test-cjs-invalid-exports');
+    const spec = 'cjs-invalid-exports';
+
+    const {
+      importMap: {imports},
+    } = await install([spec], {
+      cwd,
+      dest,
+    });
+
+    const output = fs.readFileSync(path.join(dest, `${spec}.js`), 'utf8');
+    expect(output).toEqual(
+      // This shouldn't contain named exports
+      expect.not.stringContaining(`export {`),
+    );
+  });
+
+  it('should convert package with valid identifiers as exports', async () => {
+    const cwd = __dirname;
+    const dest = path.join(cwd, 'test-cjs-valid-exports');
+    const spec = 'cjs-valid-exports';
+
+    const {
+      importMap: {imports},
+    } = await install([spec], {
+      cwd,
+      dest,
+    });
+
+    const output = fs.readFileSync(path.join(dest, `${spec}.js`), 'utf8');
+    expect(output).toEqual(
+      // Correctly exports the valid identifiers as tree-shakeable identifiers
+      expect.stringContaining(`export { entrypoint as __moduleExports, a, b, d };`),
+    );
+  });
+});

test/esinstall/cjs-autodetect-exports/package.json

@@ -0,0 +1,9 @@
+{
+  "private": true,
+  "version": "1.0.1",
+  "name": "@snowpack/test-autodetect-cjs",
+  "dependencies": {
+    "cjs-invalid-exports": "file:./packages/cjs-invalid-exports",
+    "cjs-valid-exports": "file:./packages/cjs-valid-exports"
+  }
+}

test/esinstall/cjs-autodetect-exports/packages/cjs-invalid-exports/entrypoint.js

@@ -0,0 +1,7 @@
+
+var mod = {
+  "a": "b",
+  ")": "ooops, this is an invalid identifier"
+};
+
+module.exports = mod;

test/esinstall/cjs-autodetect-exports/packages/cjs-invalid-exports/package.json

@@ -0,0 +1,5 @@
+{
+  "version": "1.0.0",
+  "name": "cjs-invalid-exports",
+  "main": "entrypoint.js"
+}

test/esinstall/cjs-autodetect-exports/packages/cjs-valid-exports/entrypoint.js

@@ -0,0 +1,8 @@
+
+var mod = {
+  "a": "b",
+  "b": "c",
+  "d": "see, these are all valid"
+};
+
+module.exports = mod;

test/esinstall/cjs-autodetect-exports/packages/cjs-valid-exports/package.json

@@ -0,0 +1,5 @@
+{
+  "version": "1.0.0",
+  "name": "cjs-valid-exports",
+  "main": "entrypoint.js"
+}

yarn.lock

@@ -3852,7 +3852,7 @@ [email protected], assert-plus@^1.0.0:
   resolved "https://registry.yarnpkg.com/assert-plus/-/assert-plus-1.0.0.tgz#f12e0f3c5d77b0b1cdd9146942e4e96c1e4dd525"
   integrity sha1-8S4PPF13sLHN2RRpQuTpbB5N1SU=
 
-assert@^1.1.1:
+assert@^1.1.1, assert@^1.4.1:
   version "1.5.0"
   resolved "https://registry.yarnpkg.com/assert/-/assert-1.5.0.tgz#55c109aaf6e0aefdb3dc4b71240c70bf574b18eb"
   integrity sha512-EDsgawzwoun2CZkCgtxJbv392v4nbk9XDD06zI+kQYoBM/3RBWLlEyJARDOmhAAosBjWACEkKL6S+lIZtcAubA==
@@ -4913,6 +4913,9 @@ cipher-base@^1.0.0, cipher-base@^1.0.1, cipher-base@^1.0.3:
     inherits "^2.0.1"
     safe-buffer "^5.0.1"
 
+"cjs-invalid-exports@file:./test/esinstall/cjs-autodetect-exports/packages/cjs-invalid-exports":
+  version "1.0.0"
+
 cjs-module-lexer@^0.6.0:
   version "0.6.0"
   resolved "https://registry.yarnpkg.com/cjs-module-lexer/-/cjs-module-lexer-0.6.0.tgz#4186fcca0eae175970aee870b9fe2d6cf8d5655f"
@@ -4932,6 +4935,9 @@ cjs-module-lexer@^1.0.0:
 "cjs-named-exports-simple@file:./test/esinstall/named-exports/packages/cjs-named-exports-simple":
   version "1.2.3"
 
+"cjs-valid-exports@file:./test/esinstall/cjs-autodetect-exports/packages/cjs-valid-exports":
+  version "1.0.0"
+
 class-utils@^0.3.5:
   version "0.3.6"
   resolved "https://registry.yarnpkg.com/class-utils/-/class-utils-0.3.6.tgz#f93369ae8b9a7ce02fd41faad0ca83033190c463"
@@ -8323,6 +8329,13 @@ is-utf8@^0.2.0:
   resolved "https://registry.yarnpkg.com/is-utf8/-/is-utf8-0.2.1.tgz#4b0da1442104d1b336340e80797e865cf39f7d72"
   integrity sha1-Sw2hRCEE0bM2NA6AeX6GXPOffXI=
 
+is-valid-identifier@^2.0.2:
+  version "2.0.2"
+  resolved "https://registry.yarnpkg.com/is-valid-identifier/-/is-valid-identifier-2.0.2.tgz#146d9dbf29821b8118580b039d2203aa4bd1da4b"
+  integrity sha512-mpS5EGqXOwzXtKAg6I44jIAqeBfntFLxpAth1rrKbxtKyI6LPktyDYpHBI+tHlduhhX/SF26mFXmxQu995QVqg==
+  dependencies:
+    assert "^1.4.1"
+
 is-whitespace@^0.3.0:
   version "0.3.0"
   resolved "https://registry.yarnpkg.com/is-whitespace/-/is-whitespace-0.3.0.tgz#1639ecb1be036aec69a54cbb401cfbed7114ab7f"
@@ -9002,6 +9015,11 @@ junk@^1.0.1:
   resolved "https://registry.yarnpkg.com/junk/-/junk-1.0.3.tgz#87be63488649cbdca6f53ab39bec9ccd2347f592"
   integrity sha1-h75jSIZJy9ym9Tqzm+yczSNH9ZI=
 
+[email protected]:
+  version "1.1.0"
+  resolved "https://registry.yarnpkg.com/keyboard-key/-/keyboard-key-1.1.0.tgz#6f2e8e37fa11475bb1f1d65d5174f1b35653f5b7"
+  integrity sha512-qkBzPTi3rlAKvX7k0/ub44sqOfXeLc/jcnGGmj5c7BJpU8eDrEVPyhCvNYAaoubbsLm9uGWwQJO1ytQK1a9/dQ==
+
 keygrip@~1.1.0:
   version "1.1.0"
   resolved "https://registry.yarnpkg.com/keygrip/-/keygrip-1.1.0.tgz#871b1681d5e159c62a445b0c74b615e0917e7226"
@@ -10066,7 +10084,7 @@ node-addon-api@^1.7.1:
   resolved "https://registry.yarnpkg.com/node-addon-api/-/node-addon-api-1.7.2.tgz#3df30b95720b53c24e59948b49532b662444f54d"
   integrity sha512-ibPK3iA+vaY1eEjESkQkM0BbCqFOaZMiXRTtdB0u7b4djtY6JnsjvPdUHVMg6xQt3B8fpTTWHI9A+ADjM9frzg==
 
-"node-builtin-pkg@file:./test/esinstall/config-polyfill-node/packages/node-builtin-pkg":
+"node-builtin-pkg@file:./test/esinstall/polyfill-node/packages/node-builtin-pkg":
   version "1.2.3"
 
 "node-env-mock-pkg@file:./test/esinstall/node-env/packages/node-env-mock-pkg":