fix(commonjs)!: fix interop when importing CJS that is transpiled ESM…

… from an actual ESM (#501)

* fix(commonjs): create more efficient dynamic require code

* chore(commonjs): update dependencies

BREAKING CHANGES: updates rollup peerDependency to ^2.22.0

* feat(commonjs): support all styles of transpiled ES modules

BREAKING CHANGES: CJS entry points will always have a default export

packages/babel/test/fixtures/named-function-helper/_config.js

@@ -1,4 +1,3 @@
 module.exports = {
-  solo: true,
   description: 'correctly renames helpers'
 };

packages/commonjs/package.json

@@ -46,33 +46,29 @@
     "require"
   ],
   "peerDependencies": {
-    "rollup": "^2.3.4"
+    "rollup": "^2.22.0"
   },
   "dependencies": {
-    "@rollup/pluginutils": "^3.0.8",
+    "@rollup/pluginutils": "^3.1.0",
     "commondir": "^1.0.1",
-    "estree-walker": "^1.0.1",
-    "glob": "^7.1.2",
-    "is-reference": "^1.1.2",
-    "magic-string": "^0.25.2",
-    "resolve": "^1.11.0"
+    "estree-walker": "^2.0.1",
+    "glob": "^7.1.6",
+    "is-reference": "^1.2.1",
+    "magic-string": "^0.25.7",
+    "resolve": "^1.17.0"
   },
   "devDependencies": {
-    "@babel/core": "^7.9.0",
-    "@babel/preset-env": "^7.9.0",
-    "@babel/register": "^7.9.0",
-    "@rollup/plugin-json": "^4.0.1",
-    "@rollup/plugin-node-resolve": "^7.0.0",
-    "acorn": "^7.1.1",
+    "@rollup/plugin-json": "^4.1.0",
+    "@rollup/plugin-node-resolve": "^8.4.0",
+    "acorn": "^7.3.1",
     "locate-character": "^2.0.5",
     "prettier": "^1.19.1",
     "require-relative": "^0.8.7",
-    "rollup": "^2.3.4",
-    "rollup-plugin-babel": "^4.3.3",
+    "rollup": "^2.22.0",
     "shx": "^0.3.2",
     "source-map": "^0.6.1",
-    "source-map-support": "^0.5.16",
-    "typescript": "^3.7.4"
+    "source-map-support": "^0.5.19",
+    "typescript": "^3.9.7"
   },
   "ava": {
     "files": [

packages/commonjs/rollup.config.js

@@ -1,25 +1,10 @@
-import babel from 'rollup-plugin-babel';
 import json from '@rollup/plugin-json';
 
 import pkg from './package.json';
 
 export default {
   input: 'src/index.js',
-  plugins: [
-    json(),
-    babel({
-      presets: [
-        [
-          '@babel/preset-env',
-          {
-            targets: {
-              node: 6
-            }
-          }
-        ]
-      ]
-    })
-  ],
+  plugins: [json()],
   external: Object.keys(pkg.dependencies).concat(['fs', 'path']),
   output: [
     {
@@ -30,6 +15,7 @@ export default {
     {
       file: pkg.main,
       format: 'cjs',
+      exports: 'auto',
       sourcemap: true
     }
   ]

packages/commonjs/src/helpers.js

@@ -20,9 +20,21 @@ export const HELPERS_ID = '\0commonjsHelpers.js';
 
 // `x['default']` is used instead of `x.default` for backward compatibility with ES3 browsers.
 // Minifiers like uglify will usually transpile it back if compatibility with ES3 is not enabled.
+// This will no longer be necessary once Rollup switches to ES6 output, likely
+// in Rollup 3
+
+// The "hasOwnProperty" call in "getDefaultExportFromCjs" is technically not
+// needed, but for consumers that use Rollup's old interop pattern, it will fix
+// rollup/rollup-plugin-commonjs#224
+// We should remove it once Rollup core and this plugin are updated to not use
+// this pattern any more
 export const HELPERS = `
 export var commonjsGlobal = typeof globalThis !== 'undefined' ? globalThis : typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : {};
 
+export function getDefaultExportFromCjs (x) {
+	return x && x.__esModule && Object.prototype.hasOwnProperty.call(x, 'default') ? x['default'] : x;
+}
+
 export function createCommonjsModule(fn, basedir, module) {
 	return module = {
 	  path: basedir,

packages/commonjs/src/index.js

@@ -80,14 +80,10 @@ export default function commonjs(options = {}) {
       return null;
     }
 
-    const moduleInfo = this.getModuleInfo(id);
-
     const transformed = transformCommonjs(
       this.parse,
       code,
       id,
-      moduleInfo.isEntry,
-      moduleInfo.importers && moduleInfo.importers.length > 0,
       isEsModule,
       ignoreGlobal || isEsModule,
       ignoreRequire,
@@ -141,6 +137,11 @@ export default function commonjs(options = {}) {
         return code;
       }
 
+      // commonjsHelpers?commonjsRegister
+      if (id.startsWith(HELPERS_ID)) {
+        return `export {${id.split('?')[1]} as default} from '${HELPERS_ID}';`;
+      }
+
       // generate proxy modules
       if (id.endsWith(EXTERNAL_SUFFIX)) {
         const actualId = getIdFromExternalProxyId(id);
@@ -154,7 +155,7 @@ export default function commonjs(options = {}) {
       }
 
       if (id === DYNAMIC_PACKAGES_ID) {
-        let code = `const { commonjsRegister } = require('${HELPERS_ID}');`;
+        let code = `const commonjsRegister = require('${HELPERS_ID}?commonjsRegister');`;
         for (const dir of dynamicRequireModuleDirPaths) {
           let entryPoint = 'index.js';
 
@@ -187,7 +188,7 @@ export default function commonjs(options = {}) {
       const normalizedPath = normalizePathSlashes(actualId);
 
       if (isDynamicJson) {
-        return `require('${HELPERS_ID}').commonjsRegister(${JSON.stringify(
+        return `const commonjsRegister = require('${HELPERS_ID}?commonjsRegister');\ncommonjsRegister(${JSON.stringify(
           getVirtualPathForDynamicRequirePath(normalizedPath, commonDir)
         )}, function (module, exports) {
   module.exports = require(${JSON.stringify(normalizedPath)});
@@ -198,7 +199,7 @@ export default function commonjs(options = {}) {
         // Try our best to still export the module fully.
         // The commonjs polyfill should take care of circular references.
 
-        return `require('${HELPERS_ID}').commonjsRegister(${JSON.stringify(
+        return `const commonjsRegister = require('${HELPERS_ID}?commonjsRegister');\ncommonjsRegister(${JSON.stringify(
           getVirtualPathForDynamicRequirePath(normalizedPath, commonDir)
         )}, function (module, exports) {
   ${readFileSync(normalizedPath, { encoding: 'utf8' })}

packages/commonjs/src/resolve-id.js

@@ -49,7 +49,7 @@ export default function getResolveId(extensions) {
       importee = getIdFromProxyId(importee);
     } else if (importee.startsWith('\0')) {
       if (
-        importee === HELPERS_ID ||
+        importee.startsWith(HELPERS_ID) ||
         importee === DYNAMIC_PACKAGES_ID ||
         importee.startsWith(DYNAMIC_JSON_PREFIX)
       ) {

packages/commonjs/src/transform.js

@@ -105,8 +105,6 @@ export function transformCommonjs(
   parse,
   code,
   id,
-  isEntry,
-  hasImporters,
   isEsModule,
   ignoreGlobal,
   ignoreRequire,
@@ -577,7 +575,7 @@ export function transformCommonjs(
   let wrapperEnd = '';
 
   const moduleName = deconflict(scope, globals, getName(id));
-  if ((!isEntry || hasImporters) && !isEsModule) {
+  if (!isEsModule) {
     const exportModuleExports = {
       str: `export { ${moduleName} as __moduleExports };`,
       name: '__moduleExports'
@@ -647,38 +645,37 @@ export function transformCommonjs(
       }
     }
 
-    if (!hasDefaultExport && (names.length || ((!isEntry || hasImporters) && !isEsModule))) {
+    if (!(isEsModule || hasDefaultExport)) {
       wrapperEnd = `\n\nvar ${moduleName} = {\n${names
         .map(({ name, deconflicted }) => `\t${name}: ${deconflicted}`)
         .join(',\n')}\n};`;
     }
   }
 
-  const defaultExport = `export default ${moduleName};`;
-
-  const named = namedExportDeclarations
-    .filter((x) => x.name !== 'default' || !hasDefaultExport)
-    .map((x) => x.str);
-
-  const exportBlock = `\n\n${(isEsModule ? [] : [defaultExport])
-    .concat(named)
-    .concat(hasDefaultExport ? defaultExportPropertyAssignments : [])
-    .join('\n')}`;
-
   magicString
     .trim()
     .prepend(importBlock + wrapperStart)
     .trim()
     .append(wrapperEnd);
 
-  const injectExportBlock =
-    hasDefaultExport || named.length > 0 || shouldWrap || !isEntry || hasImporters;
-  if (injectExportBlock) {
-    magicString.append(exportBlock);
-  }
+  const defaultExport =
+    code.indexOf('__esModule') >= 0
+      ? `export default /*@__PURE__*/${HELPERS_NAME}.getDefaultExportFromCjs(${moduleName});`
+      : `export default ${moduleName};`;
+
+  const named = namedExportDeclarations
+    .filter((x) => x.name !== 'default' || !hasDefaultExport)
+    .map((x) => x.str);
+
+  magicString.append(
+    `\n\n${(isEsModule ? [] : [defaultExport])
+      .concat(named)
+      .concat(hasDefaultExport ? defaultExportPropertyAssignments : [])
+      .join('\n')}`
+  );
 
   code = magicString.toString();
   const map = sourceMap ? magicString.generateMap() : null;
 
-  return { code, map, syntheticNamedExports: injectExportBlock };
+  return { code, map, syntheticNamedExports: '__moduleExports' };
 }

packages/commonjs/test/fixtures/form/no-exports-entry/output.js

@@ -6,3 +6,10 @@ var foo = function () {
 };
 
 var input = 42;
+
+var input_1 = {
+
+};
+
+export default input_1;
+export { input_1 as __moduleExports };

packages/commonjs/test/fixtures/function/dynamic-require-instances/_config.js

@@ -1,4 +1,4 @@
-const nodeResolve = require('@rollup/plugin-node-resolve');
+const { nodeResolve } = require('@rollup/plugin-node-resolve');
 
 module.exports = {
   description: 'returns the same module instance if required directly or via package.json/index.js',

packages/commonjs/test/fixtures/function/dynamic-require-package-sub/_config.js

@@ -1,10 +1,12 @@
+const path = require('path');
+
 const json = require('@rollup/plugin-json');
-const nodeResolve = require('@rollup/plugin-node-resolve');
+const { nodeResolve } = require('@rollup/plugin-node-resolve');
 
 module.exports = {
-  input: 'sub/entry.js',
   description: 'resolves imports of node_modules from subdirectories',
   options: {
+    input: path.join(__dirname, 'sub/entry.js'),
     plugins: [nodeResolve(), json()]
   },
   pluginOptions: {

packages/commonjs/test/fixtures/function/dynamic-require-package/_config.js

@@ -1,5 +1,5 @@
 const json = require('@rollup/plugin-json');
-const nodeResolve = require('@rollup/plugin-node-resolve');
+const { nodeResolve } = require('@rollup/plugin-node-resolve');
 
 module.exports = {
   description: 'resolves imports of directories via package.json files',

packages/commonjs/test/fixtures/function/dynamic-require-resolve-index/_config.js

@@ -1,4 +1,4 @@
-const nodeResolve = require('@rollup/plugin-node-resolve');
+const { nodeResolve } = require('@rollup/plugin-node-resolve');
 
 module.exports = {
   description: 'resolves imports of directories via index.js',

packages/commonjs/test/fixtures/function/es6-export-with-global-sniffing/_config.js

@@ -1,4 +1,4 @@
-const nodeResolve = require('@rollup/plugin-node-resolve');
+const { nodeResolve } = require('@rollup/plugin-node-resolve');
 
 module.exports = {
   options: {

packages/commonjs/test/fixtures/function/late-entry-dependency/_config.js

@@ -0,0 +1,23 @@
+const path = require('path');
+const fs = require('fs');
+
+const ID_MAIN = path.join(__dirname, 'main.js');
+const ID_OTHER = path.join(__dirname, 'other.js');
+
+module.exports = {
+  options: {
+    input: [ID_MAIN, ID_OTHER],
+    plugins: [
+      {
+        load(id) {
+          if (id === ID_MAIN) {
+            return new Promise((resolve) =>
+              setTimeout(() => resolve(fs.readFileSync(ID_MAIN, 'utf8')), 100)
+            );
+          }
+          return null;
+        }
+      }
+    ]
+  }
+};

packages/commonjs/test/fixtures/function/late-entry-dependency/main.js

@@ -0,0 +1,3 @@
+const foo = require('./other.js');
+
+t.is(foo, 'foo');

packages/commonjs/test/fixtures/function/late-entry-dependency/other.js

@@ -0,0 +1 @@
+module.exports = 'foo';

packages/commonjs/test/fixtures/function/no-exports-entry/dep.js

@@ -0,0 +1 @@
+module.exports = 42;

packages/commonjs/test/fixtures/function/no-exports-entry/main.js

@@ -0,0 +1,3 @@
+const dep = require('./dep.js');
+
+t.is(dep, 42);

packages/commonjs/test/fixtures/function/transpiled-esm-default/dep.js

@@ -0,0 +1,2 @@
+Object.defineProperty(exports, '__esModule', { value: true });
+exports.default = 'default';

packages/commonjs/test/fixtures/function/transpiled-esm-default/main.js

@@ -0,0 +1,3 @@
+import dep from './dep';
+
+t.is(dep, 'default');

packages/commonjs/test/fixtures/function/transpiled-esm-entry-default/_config.js

@@ -0,0 +1,9 @@
+const path = require('path');
+
+module.exports = {
+  description:
+    'Creates correct exports if an ES module that was transpiled to CJS is used as entry point',
+  options: {
+    input: [path.join(__dirname, 'main.js'), path.join(__dirname, 'entry.js')]
+  }
+};

packages/commonjs/test/fixtures/function/transpiled-esm-entry-default/entry.js

@@ -0,0 +1,2 @@
+Object.defineProperty(exports, '__esModule', { value: true });
+exports.default = 'default';

packages/commonjs/test/fixtures/function/transpiled-esm-entry-default/main.js

@@ -0,0 +1,3 @@
+import * as entry from './entry.js';
+
+t.deepEqual(entry, { default: 'default' });

packages/commonjs/test/fixtures/function/transpiled-esm-entry-mixed/_config.js

@@ -0,0 +1,9 @@
+const path = require('path');
+
+module.exports = {
+  description:
+    'Creates correct exports if an ES module that was transpiled to CJS is used as entry point',
+  options: {
+    input: [path.join(__dirname, 'main.js'), path.join(__dirname, 'entry.js')]
+  }
+};

packages/commonjs/test/fixtures/function/transpiled-esm-entry-mixed/entry.js

@@ -0,0 +1,3 @@
+Object.defineProperty(exports, '__esModule', { value: true });
+exports.default = 'default';
+exports.named = 'named';

packages/commonjs/test/fixtures/function/transpiled-esm-entry-mixed/main.js

@@ -0,0 +1,3 @@
+import * as entry from './entry.js';
+
+t.deepEqual(entry, { default: 'default', named: 'named' });

packages/commonjs/test/fixtures/function/transpiled-esm-entry-named/_config.js

@@ -0,0 +1,9 @@
+const path = require('path');
+
+module.exports = {
+  description:
+    'Creates correct exports if an ES module that was transpiled to CJS is used as entry point',
+  options: {
+    input: [path.join(__dirname, 'main.js'), path.join(__dirname, 'entry.js')]
+  }
+};