Add feature flag to throw on empty files

MonicaOlejniczak · MonicaOlejniczak · commit b82eab582055 · 2024-08-06T14:04:38.000+10:00
diff --git a/packages/core/feature-flags/src/index.js b/packages/core/feature-flags/src/index.js
@@ -10,6 +10,7 @@ export const DEFAULT_FEATURE_FLAGS: FeatureFlags = {
   parcelV3: false,
   importRetry: false,
   ownedResolverStructures: false,
+  panicOnEmptyFileImport: false,
 };
 
 let featureFlagValues: FeatureFlags = {...DEFAULT_FEATURE_FLAGS};
diff --git a/packages/core/feature-flags/src/types.js b/packages/core/feature-flags/src/types.js
@@ -15,4 +15,9 @@ export type FeatureFlags = {|
    * Enable resolver refactor into owned data structures.
    */
   ownedResolverStructures: boolean,
+
+  /**
+   * Makes Parcel panic when an empty file is imported
+   */
+  panicOnEmptyFileImport: boolean,
 |};
diff --git a/packages/core/integration-tests/.mocharc.json b/packages/core/integration-tests/.mocharc.json
@@ -2,7 +2,5 @@
   "require": ["@parcel/babel-register", "@parcel/test-utils/src/mochaSetup.js"],
   "timeout": 50000,
   // TODO: Remove this when https://github.com/nodejs/node/pull/28788 is resolved
-  "exit": true,
-  // This helps reduce CI flakiness of the Mac OS test run
-  "retries": 2
+  "exit": true
 }
diff --git a/packages/core/integration-tests/test/integration/env-mutate/node_modules/foo/package.json b/packages/core/integration-tests/test/integration/env-mutate/node_modules/foo/package.json
diff --git a/packages/core/integration-tests/test/javascript.js b/packages/core/integration-tests/test/javascript.js
@@ -3104,6 +3104,9 @@ describe('javascript', function () {
         path.join(__dirname, '/integration/bundle-text/javascript.js'),
         {
           mode: 'production',
+          featureFlags: {
+            panicOnEmptyFileImport: false,
+          },
         },
       );
 
@@ -5548,7 +5551,12 @@ describe('javascript', function () {
               __dirname,
               '/integration/scope-hoisting/es6/side-effects-re-exports-all-empty/a.js',
             ),
-            options,
+            {
+              ...options,
+              featureFlags: {
+                panicOnEmptyFileImport: false,
+              },
+            },
           );
 
           if (usesSymbolPropagation) {
@@ -5559,6 +5567,26 @@ describe('javascript', function () {
         },
       );
 
+      it.v2(
+        'throws when deferring an unused ES6 re-export (wildcard, empty, unused)',
+        async function () {
+          await assert.rejects(
+            () =>
+              bundle(
+                path.join(
+                  __dirname,
+                  '/integration/scope-hoisting/es6/side-effects-re-exports-all-empty/a.js',
+                ),
+                options,
+              ),
+            {
+              message:
+                'integration/scope-hoisting/es6/side-effects-re-exports-all-empty/library/empty.js must export a value',
+            },
+          );
+        },
+      );
+
       it.v2(
         'supports deferring unused ES6 re-exports (reexport named used)',
         async function () {
@@ -6116,6 +6144,44 @@ describe('javascript', function () {
         },
       );
 
+      it.v2(
+        'missing exports should throw when panicOnEmptyFileImport is enabled',
+        async function () {
+          await fsFixture(overlayFS, __dirname)`
+            empty.js:
+              // intentionally empty
+
+            index.js:
+              import {a} from './empty.js';
+
+            package.json:
+              {
+                "sideEffects": false
+              }
+
+            yarn.lock: {}
+          `;
+
+          await assert.rejects(
+            () =>
+              bundle(
+                path.join(
+                  __dirname,
+                  '/integration/scope-hoisting/es6/empty-module/a.js',
+                ),
+                {
+                  ...options,
+                  inputFS: overlayFS,
+                },
+              ),
+            {
+              message:
+                'integration/scope-hoisting/es6/empty-module/b.js must export a value',
+            },
+          );
+        },
+      );
+
       it.v2(
         'supports namespace imports of theoretically excluded reexporting assets',
         async function () {
@@ -6545,7 +6611,10 @@ describe('javascript', function () {
             __dirname,
             '/integration/scope-hoisting/es6/side-effects-commonjs/a.js',
           ),
-          options,
+          {
+            ...options,
+            outputFS: inputFS,
+          },
         );
 
         if (usesSymbolPropagation) {
diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js
@@ -366,11 +366,64 @@ describe.v2('scope hoisting', function () {
           __dirname,
           'integration/scope-hoisting/es6/re-export-all-fallback-3/entry.js',
         ),
+        {
+          featureFlags: {
+            panicOnEmptyFileImport: false,
+          },
+        },
       );
       let output = await run(b);
       assert.strictEqual(output, 'FOOBAR!');
     });
 
+    it('produces an invalid bundle when there is an empty file with export *', async () => {
+      await fsFixture(overlayFS, __dirname)`
+        empty.js:
+          // intentionally empty
+
+        a.js:
+          export * from './b.js';
+          export * from './c.js';
+
+        b.js:
+          export * from './empty.js';
+          export * from './test.js';
+
+        c.js:
+          export * from './empty.js';
+
+        index.js:
+          import {test} from './a.js';
+          output(test);
+
+        package.json:
+          {
+            "sideEffects": false
+          }
+
+        test.js:
+          export const test = 'should not fail';
+
+        yarn.lock: {}
+      `;
+
+      let result = await bundle(path.join(__dirname, 'index.js'), {
+        featureFlags: {
+          panicOnEmptyFileImport: false,
+        },
+        inputFS: overlayFS,
+      });
+
+      let output;
+      await run(result, {
+        output(v) {
+          output = v;
+        },
+      });
+
+      assert.equal(output, undefined);
+    });
+
     it('supports nested re-exporting all when falling back to namespace at runtime', async function () {
       let b = await bundle(
         path.join(
@@ -390,6 +443,9 @@ describe.v2('scope hoisting', function () {
           '/integration/scope-hoisting/es6/re-export-all-empty-no-side-effects/index.js',
         ),
         {
+          featureFlags: {
+            panicOnEmptyFileImport: false,
+          },
           mode: 'production',
         },
       );
@@ -405,6 +461,25 @@ describe.v2('scope hoisting', function () {
       assert.match(contents, /output="foo bar"/);
     });
 
+    it('panics when re-exporting all from an empty module without side effects and panicOnEmptyFileImport is enabled', async function () {
+      await assert.rejects(
+        () =>
+          bundle(
+            path.join(
+              __dirname,
+              '/integration/scope-hoisting/es6/re-export-all-empty-no-side-effects/index.js',
+            ),
+            {
+              mode: 'production',
+            },
+          ),
+        {
+          message:
+            'integration/scope-hoisting/es6/re-export-all-empty-no-side-effects/node\\_modules/lib/empty.js must export a value',
+        },
+      );
+    });
+
     it('supports re-exporting all with ambiguous CJS and non-renaming and renaming dependency retargeting', async function () {
       let b = await bundle(
         path.join(
@@ -6001,6 +6076,9 @@ describe.v2('scope hoisting', function () {
 
     try {
       let b = await bundle(path.join(testDir, 'index.html'), {
+        featureFlags: {
+          panicOnEmptyFileImport: false,
+        },
         inputFS: slowFooFS,
         outputFS: slowFooFS,
         shouldDisableCache: true,
@@ -6015,6 +6093,9 @@ describe.v2('scope hoisting', function () {
       let slowBarFS = new Proxy(overlayFS, waitHandler('bar.js', 'foo.js'));
 
       let b2 = await bundle(path.join(testDir, 'index.html'), {
+        featureFlags: {
+          panicOnEmptyFileImport: false,
+        },
         inputFS: slowBarFS,
         outputFS: slowBarFS,
         shouldDisableCache: true,
diff --git a/packages/core/test-utils/src/utils.js b/packages/core/test-utils/src/utils.js
@@ -133,6 +133,7 @@ export function getParcelOptions(
         },
       },
       featureFlags: {
+        panicOnEmptyFileImport: true,
         parcelV3: isParcelV3,
       },
     },
diff --git a/packages/transformers/js/src/JSTransformer.js b/packages/transformers/js/src/JSTransformer.js
@@ -6,7 +6,6 @@ import type {
   FilePath,
   FileCreateInvalidation,
 } from '@parcel/types';
-import type {SchemaEntity} from '@parcel/utils';
 import type {Diagnostic} from '@parcel/diagnostic';
 import SourceMap from '@parcel/source-map';
 import {Transformer} from '@parcel/plugin';
@@ -18,7 +17,13 @@ import ThrowableDiagnostic, {
   encodeJSONKeyComponent,
   convertSourceLocationToHighlight,
 } from '@parcel/diagnostic';
-import {validateSchema, remapSourceLocation, globMatch} from '@parcel/utils';
+import {
+  globMatch,
+  relativePath,
+  remapSourceLocation,
+  type SchemaEntity,
+  validateSchema,
+} from '@parcel/utils';
 import pkg from '../package.json';
 
 const JSX_EXTENSIONS = {
@@ -862,6 +867,23 @@ export default (new Transformer({
 
     asset.meta.id = asset.id;
     if (hoist_result) {
+      if (
+        options.featureFlags.panicOnEmptyFileImport &&
+        !asset.pipeline &&
+        !asset.sideEffects &&
+        !hoist_result.exported_symbols.length &&
+        !hoist_result.imported_symbols.length &&
+        !hoist_result.re_exports.length
+      ) {
+        throw new Error(
+          `${relativePath(
+            options.projectRoot,
+            asset.filePath,
+            false,
+          )} must export a value`,
+        );
+      }
+
       asset.symbols.ensure();
       for (let {
         exported,
@@ -974,6 +996,23 @@ export default (new Transformer({
       asset.meta.shouldWrap = hoist_result.should_wrap;
     } else {
       if (symbol_result) {
+        if (
+          options.featureFlags.panicOnEmptyFileImport &&
+          !asset.pipeline &&
+          !asset.sideEffects &&
+          !symbol_result.exports.length &&
+          !symbol_result.exports_all.length &&
+          !symbol_result.imports.length
+        ) {
+          throw new Error(
+            `${relativePath(
+              options.projectRoot,
+              asset.filePath,
+              false,
+            )} must export a value`,
+          );
+        }
+
         let deps = new Map(
           asset
             .getDependencies()
diff --git a/packages/utils/node-resolver-core/src/_empty.js b/packages/utils/node-resolver-core/src/_empty.js
@@ -0,0 +1 @@
+module.exports = {};

Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,5 @@`
`2`	`2`	`"require": ["@parcel/babel-register", "@parcel/test-utils/src/mochaSetup.js"],`
`3`	`3`	`"timeout": 50000,`
`4`	`4`	`// TODO: Remove this when https://github.com/nodejs/node/pull/28788 is resolved`
`5`		`- "exit": true,`
`6`		`- // This helps reduce CI flakiness of the Mac OS test run`
`7`		`- "retries": 2`
	`5`	`+ "exit": true`
`8`	`6`	`}`