fix(shaker): do not remove args of functions if they aren't used (#1098)

callstack · Oct 25, 2022 · 963508a · 963508a
1 parent a488dae
commit 963508a
Show file tree

Hide file tree

Showing 7 changed files with 157 additions and 86 deletions.
diff --git a/.changeset/many-beers-accept.md b/.changeset/many-beers-accept.md
@@ -0,0 +1,7 @@
+---
+'@linaria/shaker': patch
+'@linaria/testkit': patch
+'@linaria/utils': patch
+---
+
+Shaker shouldn't remove parameters of functions if they aren't used.
diff --git a/packages/shaker/src/plugins/__tests__/__snapshots__/shaker-plugin.test.ts.snap b/packages/shaker/src/plugins/__tests__/__snapshots__/shaker-plugin.test.ts.snap
@@ -1,5 +1,7 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`shaker should delete import 1`] = `"export const Alive = \\"\\";"`;
+
 exports[`shaker should keep assigment even if export is marked for removing 1`] = `
 "let _a;
 

diff --git a/packages/shaker/src/plugins/__tests__/shaker-plugin.test.ts b/packages/shaker/src/plugins/__tests__/shaker-plugin.test.ts
@@ -179,4 +179,21 @@ describe('shaker', () => {
     expect(code).toMatchSnapshot();
     expect(metadata.imports.size).toBe(0);
   });
+
+  it('should delete import', () => {
+    const { code, metadata } = keep(['Alive'])`
+      import { A, B } from "ABC";
+      const AB = {
+        A,
+        B,
+      };
+
+      export const Alive = "";
+
+      export default AB;
+    `;
+
+    expect(code).toMatchSnapshot();
+    expect(metadata.imports.size).toBe(0);
+  });
 });
diff --git a/packages/shaker/src/plugins/shaker-plugin.ts b/packages/shaker/src/plugins/shaker-plugin.ts
@@ -17,8 +17,9 @@ import {
   removeWithRelated,
   sideEffectImport,
   reference,
-  findParentForDelete,
+  findActionForNode,
   dereference,
+  mutate,
 } from '@linaria/utils';
 
 type Core = typeof core;
@@ -257,7 +258,8 @@ export default function shakerPlugin(
           // eslint-disable-next-line no-restricted-syntax
           for (const path of forDeleting) {
             const binding = getBindingForExport(path);
-            const parent = findParentForDelete(path);
+            const action = findActionForNode(path);
+            const parent = action?.[1];
             const outerReferences = (binding?.referencePaths || []).filter(
               (ref) => ref !== parent && !parent?.isAncestor(ref)
             );
@@ -271,7 +273,20 @@ export default function shakerPlugin(
               !deleted.has(path) &&
               (!binding || outerReferences.length === 0)
             ) {
-              removeWithRelated([parent ?? path]);
+              if (action) {
+                mutate(action[1], (p) => {
+                  if (isRemoved(p)) return;
+
+                  if (action[0] === 'remove') {
+                    p.remove();
+                  } else if (action[0] === 'replace') {
+                    p.replaceWith(action[2]);
+                  }
+                });
+              } else {
+                removeWithRelated([path]);
+              }
+
               deleted.add(path);
               changed = true;
             }

diff --git a/packages/testkit/src/utils/__snapshots__/removeWithRelated.test.ts.snap b/packages/testkit/src/utils/__snapshots__/removeWithRelated.test.ts.snap
@@ -10,13 +10,16 @@ exports[`removeWithRelated should keep logical expression 1`] = `
 const res = false && c;"
 `;
 
-exports[`removeWithRelated should remove export 1`] = `
-"const b = 1;
-export { b };"
+exports[`removeWithRelated should not delete params of functions 1`] = `
+"function test(arg) {
+  return null;
+}"
 `;
 
 exports[`removeWithRelated should remove node if it becomes invalid after removing its children 1`] = `""`;
 
+exports[`removeWithRelated should remove the whole import 1`] = `""`;
+
 exports[`removeWithRelated should remove try/catch block 1`] = `"const a = 1;"`;
 
 exports[`removeWithRelated should shake try/catch 1`] = `""`;
diff --git a/packages/testkit/src/utils/removeWithRelated.test.ts b/packages/testkit/src/utils/removeWithRelated.test.ts
@@ -57,34 +57,24 @@ describe('removeWithRelated', () => {
     expect(code).toMatchSnapshot();
   });
 
-  it('should remove try/catch block', () => {
+  it('should remove the whole import', () => {
     const code = run`
-      const a = 1;
+      import { a } from './source';
 
-      try {
-        /* remove */42;
-      } catch (e) {
-      }
+      /* remove */a;
     `;
 
     expect(code).toMatchSnapshot();
   });
 
-  it('should remove export', () => {
+  it('should remove try/catch block', () => {
     const code = run`
-      function a() {
-        /* remove */42;
-      }
+      const a = 1;
 
-      function checkIsBrowser() {
-        return /* remove */42;
+      try {
+        /* remove */42;
+      } catch (e) {
       }
-
-      const canUseDOM = checkIsBrowser();
-
-      const b = 1;
-
-      export { a, b, canUseDOM };
     `;
 
     expect(code).toMatchSnapshot();
@@ -123,7 +113,19 @@ describe('removeWithRelated', () => {
     const code = run`
       /* remove */const mode = "DEV";
 
-      export { mode };
+      if (mode !== "DEV") {
+      }
+    `;
+
+    expect(code).toMatchSnapshot();
+  });
+
+  it('should not delete params of functions', () => {
+    const code = run`
+      function test(arg) {
+        /* remove */console.log(arg);
+        return null;
+      }
     `;
 
     expect(code).toMatchSnapshot();