fix(node-resolve): preserve moduleSideEffects when re-resolving files (…

…#1245)

BREAKING CHANGES: Requires Rollup>=2.78.0

packages/node-resolve/package.json

@@ -52,7 +52,7 @@
     "modules"
   ],
   "peerDependencies": {
-    "rollup": "^2.42.0"
+    "rollup": "^2.78.0"
   },
   "dependencies": {
     "@rollup/pluginutils": "^3.1.0",
@@ -66,10 +66,10 @@
     "@babel/core": "^7.10.5",
     "@babel/plugin-transform-typescript": "^7.10.5",
     "@rollup/plugin-babel": "^5.1.0",
-    "@rollup/plugin-commonjs": "^16.0.0",
+    "@rollup/plugin-commonjs": "^22.0.2",
     "@rollup/plugin-json": "^4.1.0",
     "es5-ext": "^0.10.53",
-    "rollup": "^2.67.3",
+    "rollup": "^2.78.1",
     "source-map": "^0.7.3",
     "string-capitalize": "^1.0.1"
   },

packages/node-resolve/src/index.js

@@ -82,7 +82,7 @@ export function nodeResolve(opts = {}) {
   const browserMapCache = new Map();
   let preserveSymlinks;
 
-  const doResolveId = async (context, importee, importer, custom) => {
+  const resolveLikeNode = async (context, importee, importer, custom) => {
     // strip query params from import
     const [importPath, params] = importee.split('?');
     const importSuffix = `${params ? `?${params}` : ''}`;
@@ -257,39 +257,52 @@ export function nodeResolve(opts = {}) {
       isDirCached.clear();
     },
 
-    async resolveId(importee, importer, resolveOptions) {
-      if (importee === ES6_BROWSER_EMPTY) {
-        return importee;
-      }
-      // ignore IDs with null character, these belong to other plugins
-      if (/\0/.test(importee)) return null;
+    resolveId: {
+      order: 'post',
+      async handler(importee, importer, resolveOptions) {
+        if (importee === ES6_BROWSER_EMPTY) {
+          return importee;
+        }
+        // ignore IDs with null character, these belong to other plugins
+        if (/\0/.test(importee)) return null;
 
-      if (/\0/.test(importer)) {
-        importer = undefined;
-      }
+        const { custom = {} } = resolveOptions;
+        const { 'node-resolve': { resolved: alreadyResolved } = {} } = custom;
+        if (alreadyResolved) {
+          return alreadyResolved;
+        }
 
-      const resolved = await doResolveId(this, importee, importer, resolveOptions.custom);
-      if (resolved) {
-        const resolvedResolved = await this.resolve(
-          resolved.id,
-          importer,
-          Object.assign({ skipSelf: true }, resolveOptions)
-        );
-        if (resolvedResolved) {
-          // Handle plugins that manually make the result external
-          if (resolvedResolved.external) {
-            return false;
-          }
-          // Allow other plugins to take over resolution. Rollup core will not
-          // change the id if it corresponds to an existing file
-          if (resolvedResolved.id !== resolved.id) {
-            return resolvedResolved;
+        if (/\0/.test(importer)) {
+          importer = undefined;
+        }
+
+        const resolved = await resolveLikeNode(this, importee, importer, custom);
+        if (resolved) {
+          // This way, plugins may attach additional meta information to the
+          // resolved id or make it external. We do not skip node-resolve here
+          // because another plugin might again use `this.resolve` in its
+          // `resolveId` hook, in which case we want to add the correct
+          // `moduleSideEffects` information.
+          const resolvedResolved = await this.resolve(resolved.id, importer, {
+            ...resolveOptions,
+            custom: { ...custom, 'node-resolve': { resolved } }
+          });
+          if (resolvedResolved) {
+            // Handle plugins that manually make the result external
+            if (resolvedResolved.external) {
+              return false;
+            }
+            // Allow other plugins to take over resolution. Rollup core will not
+            // change the id if it corresponds to an existing file
+            if (resolvedResolved.id !== resolved.id) {
+              return resolvedResolved;
+            }
+            // Pass on meta information added by other plugins
+            return { ...resolved, meta: resolvedResolved.meta };
           }
-          // Pass on meta information added by other plugins
-          return { ...resolved, meta: resolvedResolved.meta };
         }
+        return resolved;
       }
-      return resolved;
     },
 
     load(importee) {

packages/node-resolve/test/side-effects.js

@@ -0,0 +1,137 @@
+import { join } from 'path';
+
+import test from 'ava';
+import { rollup } from 'rollup';
+
+import commonjs from '@rollup/plugin-commonjs';
+
+import { nodeResolve } from '..';
+import { getCode, testBundle } from '../../../util/test';
+
+process.chdir(join(__dirname, 'fixtures'));
+
+const failOnWarn = (t) => (warning) =>
+  t.fail(`No warnings were expected, got:\n${warning.code}\n${warning.message}`);
+
+test('respects the package.json sideEffects property for files in root package by default', async (t) => {
+  const bundle = await rollup({
+    input: 'root-package-side-effect/index.js',
+    onwarn: failOnWarn(t),
+    plugins: [
+      nodeResolve({
+        rootDir: 'root-package-side-effect'
+      })
+    ]
+  });
+
+  const code = await getCode(bundle);
+  t.false(code.includes('side effect'));
+  t.snapshot(code);
+});
+
+test('respects the package.json sideEffects when commonjs plugin is used', async (t) => {
+  const bundle = await rollup({
+    input: 'root-package-side-effect/index.js',
+    onwarn: failOnWarn(t),
+    plugins: [
+      commonjs(),
+      nodeResolve({
+        rootDir: 'root-package-side-effect'
+      })
+    ]
+  });
+
+  const code = await getCode(bundle);
+  t.false(code.includes('side effect'));
+  t.snapshot(code);
+});
+
+test('respects the package.json sideEffects when when another plugin uses this.load it its resolveId hook', async (t) => {
+  const bundle = await rollup({
+    input: 'root-package-side-effect/index.js',
+    onwarn: failOnWarn(t),
+    plugins: [
+      {
+        name: 'test',
+        async resolveId(source, importer, resolveOptions) {
+          const resolved = await this.resolve(source, importer, {
+            ...resolveOptions,
+            skipSelf: true
+          });
+          // This starts loading the module and fixes the value of
+          // `moduleSideEffects` with whatever is contained in "resolved"
+          await this.load(resolved);
+          return resolved;
+        }
+      },
+      nodeResolve({
+        rootDir: 'root-package-side-effect'
+      })
+    ]
+  });
+
+  const code = await getCode(bundle);
+  t.false(code.includes('side effect'));
+  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',
+    onwarn: failOnWarn(t),
+    plugins: [
+      nodeResolve({
+        rootDir: 'root-package-side-effect',
+        ignoreSideEffectsForRoot: true
+      })
+    ]
+  });
+
+  const code = await getCode(bundle);
+  t.true(code.includes('side effect'));
+  t.snapshot(code);
+});
+
+test('handles package side-effects', async (t) => {
+  const bundle = await rollup({
+    input: 'side-effects.js',
+    onwarn: failOnWarn(t),
+    plugins: [nodeResolve()]
+  });
+  await testBundle(t, bundle);
+  t.snapshot(global.sideEffects);
+
+  delete global.sideEffects;
+});

packages/node-resolve/test/snapshots/side-effects.js.md

@@ -0,0 +1,82 @@
+# Snapshot report for `test/side-effects.js`
+
+The actual snapshot is saved in `side-effects.js.snap`.
+
+Generated by [AVA](https://avajs.dev).
+
+## respects the package.json sideEffects property for files in root package by default
+
+> Snapshot 1
+
+    `'use strict';␊
+    ␊
+    console.log('main');␊
+    `
+
+## respects the package.json sideEffects when commonjs plugin is used
+
+> Snapshot 1
+
+    `'use strict';␊
+    ␊
+    console.log('main');␊
+    `
+
+## respects the package.json sideEffects when when another plugin uses this.load it its resolveId hook
+
+> Snapshot 1
+
+    `'use strict';␊
+    ␊
+    console.log('main');␊
+    `
+
+## 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');␊
+    `
+
+## ignores the package.json sideEffects property for files in root package with "ignoreSideEffectsForRoot" option
+
+> Snapshot 1
+
+    `'use strict';␊
+    ␊
+    console.log('side effect');␊
+    ␊
+    console.log('main');␊
+    `
+
+## handles package side-effects
+
+> Snapshot 1
+
+    [
+      'array-dep1',
+      'array-dep3',
+      'array-dep5',
+      'array-index',
+      'false-dep1',
+      'true-dep1',
+      'true-dep2',
+      'true-index',
+    ]