fix(shaker): workaround for weirdly packaged cjs modules (#1178)

* fix: workaround for weirdly packaged cjs modules * chore: changeset for #1178 Co-authored-by: Victor Lin <victor.lin@airbnb.com> Co-authored-by: Anton Evzhakov <anton@evz.name>
callstack · Jan 17, 2023 · 71a5b35 · 71a5b35
1 parent b27f328
commit 71a5b35
Show file tree

Hide file tree

Showing 6 changed files with 142 additions and 15 deletions.
diff --git a/.changeset/shiny-owls-visit.md b/.changeset/shiny-owls-visit.md
@@ -0,0 +1,7 @@
+---
+'@linaria/shaker': patch
+'@linaria/testkit': patch
+'@linaria/utils': patch
+---
+
+Workaround for weirdly packaged cjs modules.
diff --git a/packages/shaker/src/plugins/__tests__/shaker-plugin.test.ts b/packages/shaker/src/plugins/__tests__/shaker-plugin.test.ts
@@ -196,6 +196,7 @@ describe('shaker', () => {
     expect(code).toMatchSnapshot();
     expect(metadata.imports.size).toBe(0);
   });
+
   it('should handle object patterns in exports', () => {
     const { code } = keep(['Alive'])`
       import foo from "foo";
@@ -211,4 +212,101 @@ describe('shaker', () => {
       } = foo();"
     `);
   });
+
+  it('avoids deleting non-default exports when importing default export of a module without an __esModule: true property', () => {
+    /* without workaround, this will be transformed by shaker to:
+      const n = require('n');
+      const defaultExports = {
+        createContext: n.createContext
+      };
+      exports.default = defaultExports;
+
+      i.e, exports.createContext is deleted
+    */
+    const { code } = keep(['default'])`
+    const n = require('n');
+    const defaultExports = { createContext: n.createContext }
+    Object.defineProperty(exports, "createContext", {
+      enumerable: !0,
+      get: function() {
+          return n.createContext
+      }
+    })
+    exports.default = defaultExports;
+    `;
+
+    /*
+      this breaks babel's interopRequireDefault
+      without shaker, interopRequireDefault(<the test module>)
+      returns: {
+        default: {
+          default: { createContext: ... }
+          createContext: ...
+        }
+      }
+      The double-default is because the module does not define
+      __esModule: true, so interopRequireDefault assumes that it needs to wrap the exports in { default: ... }, so that subsequent code can access createContext via `<test module>.default.createContext`.
+
+      If shaker treats the createExport named export as unused (since it's not in onlyExports), it will delete it. interopRequireDefault(<shaker-processed test module>)
+      returns: {
+        default: {
+          default: { createContext: ... }
+        }
+      }
+
+      And `<test module>.default.createContext` will be undefined.
+
+      Therefore, we assert that createContext is not deleted in this case.
+    */
+    expect(code).toMatchInlineSnapshot(`
+      "const n = require('n');
+      const defaultExports = {
+        createContext: n.createContext
+      };
+      Object.defineProperty(exports, \\"createContext\\", {
+        enumerable: !0,
+        get: function () {
+          return n.createContext;
+        }
+      });
+      exports.default = defaultExports;"
+    `);
+  });
+
+  it('deletes non-default exports when importing default export of a module with an __esModule: true property', () => {
+    /* without workaround, this will be transformed by shaker to:
+      const n = require('n');
+      const defaultExports = {
+        createContext: n.createContext
+      };
+      exports.default = defaultExports;
+
+      i.e, exports.createContext is deleted
+    */
+    const { code } = keep(['default'])`
+    const n = require('n');
+    const defaultExports = { createContext: n.createContext }
+    Object.defineProperty(exports, "__esModule", {
+      value: true
+    });
+    Object.defineProperty(exports, "createContext", {
+      enumerable: !0,
+      get: function() {
+          return n.createContext
+      }
+    })
+    exports.default = defaultExports;
+    `;
+
+    expect(code).toMatchInlineSnapshot(`
+      "const n = require('n');
+      const defaultExports = {
+        createContext: n.createContext
+      };
+      Object.defineProperty(exports, \\"__esModule\\", {
+        value: true
+      });
+      exports.default = defaultExports;"
+    `);
+  });
 });
diff --git a/packages/shaker/src/plugins/shaker-plugin.ts b/packages/shaker/src/plugins/shaker-plugin.ts
@@ -197,7 +197,20 @@ export default function shakerPlugin(
 
         return;
       }
-
+      // Hackaround for packages which include a 'default' export without specifying __esModule; such packages cannot be
+      // shaken as they will break interopRequireDefault babel helper
+      // See example in shaker-plugin.test.ts
+      // Real-world example was found in preact/compat npm package
+      if (
+        onlyExports.includes('default') &&
+        exports.find(({ exported }) => exported === 'default') &&
+        !collected.isEsModule
+      ) {
+        this.imports = collected.imports;
+        this.exports = exports;
+        this.reexports = collected.reexports;
+        return;
+      }
       if (!onlyExports.includes('*')) {
         const aliveExports = new Set<IExport | IReexport>();
         const importNames = collected.imports.map(({ imported }) => imported);

diff --git a/packages/testkit/src/module.test.ts b/packages/testkit/src/module.test.ts
@@ -1,4 +1,5 @@
 import path from 'path';
+
 import dedent from 'dedent';
 
 import { Module, TransformCacheCollection } from '@linaria/babel-preset';

diff --git a/packages/utils/src/collectExportsAndImports.ts b/packages/utils/src/collectExportsAndImports.ts
@@ -65,6 +65,7 @@ export interface IState {
   exports: IExport[];
   imports: (IImport | ISideEffectImport)[];
   reexports: IReexport[];
+  isEsModule: boolean;
 }
 
 export const sideEffectImport = (
@@ -606,6 +607,8 @@ function collectFromExports(path: NodePath<Identifier>, state: IState): void {
 
     const { name } = property.node;
     if (name === '__esModule') {
+      // eslint-disable-next-line no-param-reassign
+      state.isEsModule = true;
       return;
     }
 
@@ -623,21 +626,25 @@ function collectFromExports(path: NodePath<Identifier>, state: IState): void {
     if (
       obj?.isIdentifier(path.node) &&
       prop?.isStringLiteral() &&
-      prop.node.value !== '__esModule' &&
       descriptor?.isObjectExpression()
     ) {
-      /**
-       *  Object.defineProperty(exports, "token", {
-       *    enumerable: true,
-       *    get: function get() {
-       *      return _unknownPackage.token;
-       *    }
-       *  });
-       */
-      const exported = prop.node.value;
-      const local = getGetterValueFromDescriptor(descriptor);
-      if (local) {
-        state.exports.push({ exported, local });
+      if (prop.node.value === '__esModule') {
+        // eslint-disable-next-line no-param-reassign
+        state.isEsModule = true;
+      } else {
+        /**
+         *  Object.defineProperty(exports, "token", {
+         *    enumerable: true,
+         *    get: function get() {
+         *      return _unknownPackage.token;
+         *    }
+         *  });
+         */
+        const exported = prop.node.value;
+        const local = getGetterValueFromDescriptor(descriptor);
+        if (local) {
+          state.exports.push({ exported, local });
+        }
       }
     } else if (
       obj?.isIdentifier(path.node) &&
@@ -1052,6 +1059,7 @@ export default function collectExportsAndImports(
     exports: [],
     imports: [],
     reexports: [],
+    isEsModule: false,
   };
 
   if (!force && cache.has(path)) {

diff --git a/packages/vite/src/index.ts b/packages/vite/src/index.ts
@@ -43,7 +43,7 @@ export default function linaria({
   // <dependency id, targets>
   const targets: { id: string; dependencies: string[] }[] = [];
   const cache = new TransformCacheCollection();
-  const { codeCache, resolveCache, evalCache } = cache;
+  const { codeCache, evalCache } = cache;
   return {
     name: 'linaria',
     enforce: 'post',