fix(node-resolve): update side effects logic to be deep when glob doesn’t contain / (#1148)

TheMcMurder · web-flow · commit 18afe15f44f0 · 2022-04-15T08:52:11.000-04:00
* feat(node-resolve): deep side effects option

* chore(node-resolve): Fixing linting errors

* feat: deep side effects - prevent changes when side effects start with "./" || "/"

* feat: support deep side effects by default - matching webpack implementation

* chore(cleanup): pr feedback
diff --git a/packages/node-resolve/src/util.js b/packages/node-resolve/src/util.js
@@ -144,7 +144,17 @@ export function getPackageInfo(options) {
     if (typeof packageSideEffects === 'boolean') {
       internalPackageInfo.hasModuleSideEffects = () => packageSideEffects;
     } else if (Array.isArray(packageSideEffects)) {
-      internalPackageInfo.hasModuleSideEffects = createFilter(packageSideEffects, null, {
+      const finalPackageSideEffects = packageSideEffects.map((sideEffect) => {
+        /*
+         * The array accepts simple glob patterns to the relevant files... Patterns like .css, which do not include a /, will be treated like **\/.css.
+         * https://webpack.js.org/guides/tree-shaking/
+         */
+        if (sideEffect.includes('/')) {
+          return sideEffect;
+        }
+        return `**/${sideEffect}`;
+      });
+      internalPackageInfo.hasModuleSideEffects = createFilter(finalPackageSideEffects, null, {
         resolve: pkgRoot
       });
     }
diff --git a/packages/node-resolve/test/fixtures/deep-side-effects-with-specific-side-effects/deep/side-effect.js b/packages/node-resolve/test/fixtures/deep-side-effects-with-specific-side-effects/deep/side-effect.js
@@ -0,0 +1 @@
+console.log('deep side effect')
diff --git a/packages/node-resolve/test/fixtures/deep-side-effects-with-specific-side-effects/index.js b/packages/node-resolve/test/fixtures/deep-side-effects-with-specific-side-effects/index.js
@@ -0,0 +1,4 @@
+import './deep/side-effect.js'
+import './shallow-side-effect.js'
+
+console.log('main')
diff --git a/packages/node-resolve/test/fixtures/deep-side-effects-with-specific-side-effects/package.json b/packages/node-resolve/test/fixtures/deep-side-effects-with-specific-side-effects/package.json
@@ -0,0 +1,4 @@
+{
+  "main": "./index.js",
+  "sideEffects": ["./*.js"]
+}
diff --git a/packages/node-resolve/test/fixtures/deep-side-effects-with-specific-side-effects/shallow-side-effect.js b/packages/node-resolve/test/fixtures/deep-side-effects-with-specific-side-effects/shallow-side-effect.js
@@ -0,0 +1 @@
+console.log('shallow side effect')
diff --git a/packages/node-resolve/test/fixtures/deep-side-effects/deep/side-effect.js b/packages/node-resolve/test/fixtures/deep-side-effects/deep/side-effect.js
@@ -0,0 +1 @@
+console.log('deep side effect')
diff --git a/packages/node-resolve/test/fixtures/deep-side-effects/index.js b/packages/node-resolve/test/fixtures/deep-side-effects/index.js
@@ -0,0 +1,4 @@
+import './deep/side-effect.js'
+import './shallow-side-effect.js'
+
+console.log('main')
diff --git a/packages/node-resolve/test/fixtures/deep-side-effects/package.json b/packages/node-resolve/test/fixtures/deep-side-effects/package.json
@@ -0,0 +1,4 @@
+{
+  "main": "./index.js",
+  "sideEffects": ["*.js"]
+}
diff --git a/packages/node-resolve/test/fixtures/deep-side-effects/shallow-side-effect.js b/packages/node-resolve/test/fixtures/deep-side-effects/shallow-side-effect.js
@@ -0,0 +1 @@
+console.log('shallow side effect')
diff --git a/packages/node-resolve/test/snapshots/test.js.md b/packages/node-resolve/test/snapshots/test.js.md
@@ -176,3 +176,27 @@ Generated by [AVA](https://avajs.dev).
     Error {
       message: 'node-resolve: `customResolveOptions.packageIterator` is no longer an option. If you need this, please open an issue.',
     }
+
+## respects the package.json sideEffects property for files in the root package and supports deep side effects
+
+> Snapshot 1
+
+    `'use strict';␊
+    ␊
+    console.log('deep side effect');␊
+    ␊
+    console.log('shallow side effect');␊
+    ␊
+    console.log('main');␊
+    `
+
+## does not prefix the sideEffects property if the side effect contains a "/"
+
+> Snapshot 1
+
+    `'use strict';␊
+    ␊
+    console.log('shallow side effect');␊
+    ␊
+    console.log('main');␊
+    `
diff --git a/packages/node-resolve/test/snapshots/test.js.snap b/packages/node-resolve/test/snapshots/test.js.snap
diff --git a/packages/node-resolve/test/test.js b/packages/node-resolve/test/test.js
@@ -322,6 +322,38 @@ test('respects the package.json sideEffects property for files in root package b
   t.snapshot(code);
 });
 
+test('respects the package.json sideEffects property for files in the root package and supports deep side effects', async (t) => {
+  const bundle = await rollup({
+    input: 'deep-side-effects/index.js',
+    onwarn: failOnWarn(t),
+    plugins: [
+      nodeResolve({
+        rootDir: 'deep-side-effects'
+      })
+    ]
+  });
+  const code = await getCode(bundle);
+  t.true(code.includes('shallow side effect'));
+  t.true(code.includes('deep side effect'));
+  t.snapshot(code);
+});
+
+test('does not prefix the sideEffects property if the side effect contains a "/"', async (t) => {
+  const bundle = await rollup({
+    input: 'deep-side-effects-with-specific-side-effects/index.js',
+    onwarn: failOnWarn(t),
+    plugins: [
+      nodeResolve({
+        rootDir: 'deep-side-effects-with-specific-side-effects'
+      })
+    ]
+  });
+  const code = await getCode(bundle);
+  t.true(code.includes('shallow side effect'));
+  t.false(code.includes('deep side effects'));
+  t.snapshot(code);
+});
+
 test('ignores the package.json sideEffects property for files in root package with "ignoreSideEffectsForRoot" option', async (t) => {
   const bundle = await rollup({
     input: 'root-package-side-effect/index.js',

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +{
 +  "main": "./index.js",
 +  "sideEffects": ["./*.js"]
 +}