reverse exports: address PR feedback

embroider-build · Nov 17, 2023 · 25e25ce · 25e25ce
1 parent 1050157
commit 25e25ce
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 26 deletions.
diff --git a/packages/reverse-exports/src/index.ts b/packages/reverse-exports/src/index.ts
@@ -77,11 +77,6 @@ export default function reversePackageExports(
   { exports: exportsObj, name }: { exports?: Exports; name: string },
   relativePath: string
 ): string {
-  // // TODO add an actual matching system and don't just look for the default
-  // if (packageJSON.exports?.['./*'] === './dist/*.js') {
-  //   return posix.join(packageJSON.name, relativePath.replace(/^.\/dist\//, `./`).replace(/\.js$/, ''));
-  // }
-
   if (!exportsObj) {
     return posix.join(name, relativePath);
   }
@@ -96,8 +91,9 @@ export default function reversePackageExports(
   });
 
   if (!maybeKeyValuePair) {
-    // TODO figure out what the result should be if it doesn't match anything in exports
-    return posix.join(name, relativePath);
+    throw new Error(
+      `You tried to reverse exports for the file \`${relativePath}\` in package \`${name}\` but it does not match any of the exports rules defined in package.json. This means it should not be possible to access directly.`
+    );
   }
 
   const { key, value } = maybeKeyValuePair;
@@ -109,7 +105,9 @@ export default function reversePackageExports(
   const maybeResolvedPaths = resolveExports({ name, exports: { [value]: key } }, relativePath);
 
   if (!maybeResolvedPaths) {
-    throw new Error('Expected path to be found at this point');
+    throw new Error(
+      `Bug Discovered! \`_findPathRecursively()\` must always return a string value but instead it found a ${typeof value}. Please report this as an issue to https://github.com/embroider-build/embroider/issues/new`
+    );
   }
 
   const [resolvedPath] = maybeResolvedPaths;

diff --git a/packages/reverse-exports/tests/reverse-exports.test.ts b/packages/reverse-exports/tests/reverse-exports.test.ts
@@ -1,23 +1,13 @@
 import reversePackageExports, { _findPathRecursively } from '../src';
 
 describe('reverse exports', function () {
-  //   it('correctly reversed exports', function () {
-  //     // TODO figure out what the result should be if it doesn't match anything in exports
-  //     expect(reversePackageExports({ name: 'best-addon' }, './dist/_app_/components/face.js')).toBe(
-  //       'best-addon/dist/_app_/components/face.js'
-  //     );
-  //     expect(
-  //       reversePackageExports(
-  //         {
-  //           name: 'best-addon',
-  //           exports: {
-  //             './*': './dist/*.js',
-  //           },
-  //         },
-  //         './dist/_app_/components/face.js'
-  //       )
-  //     ).toBe('best-addon/_app_/components/face');
-  //   });
+  it('exports is missing', function () {
+    // TODO figure out what the result should be if it doesn't match anything in exports
+    expect(reversePackageExports({ name: 'best-addon' }, './dist/_app_/components/face.js')).toBe(
+      'best-addon/dist/_app_/components/face.js'
+    );
+  });
+
   it('exports is a string', function () {
     const actual = reversePackageExports(
       {
@@ -28,6 +18,7 @@ describe('reverse exports', function () {
     );
     expect(actual).toBe('my-addon');
   });
+
   it('exports is an object with one entry', function () {
     const actual = reversePackageExports(
       {
@@ -40,6 +31,7 @@ describe('reverse exports', function () {
     );
     expect(actual).toBe('my-addon');
   });
+
   it('subpath exports', function () {
     const packageJson = {
       name: 'my-addon',
@@ -59,6 +51,7 @@ describe('reverse exports', function () {
     expect(reversePackageExports(packageJson, './yet-another/deep/file.js')).toBe('my-addon/other-prefix/deep/file');
     expect(reversePackageExports(packageJson, './grod/very/deep/file.js')).toBe('my-addon/glob/very/deep/file');
   });
+
   it('alternative exports', function () {
     const packageJson = {
       name: 'my-addon',
@@ -69,6 +62,7 @@ describe('reverse exports', function () {
     expect(reversePackageExports(packageJson, './good-things/apple.js')).toBe('my-addon/things/apple.js');
     expect(reversePackageExports(packageJson, './bad-things/apple.js')).toBe('my-addon/things/apple.js');
   });
+
   it('conditional exports - simple abbreviated', function () {
     const packageJson = {
       name: 'my-addon',
@@ -82,6 +76,7 @@ describe('reverse exports', function () {
     expect(reversePackageExports(packageJson, './index-require.cjs')).toBe('my-addon');
     expect(reversePackageExports(packageJson, './index.js')).toBe('my-addon');
   });
+
   it('conditional exports - simple non-abbreviated', function () {
     const packageJson = {
       name: 'my-addon',
@@ -97,6 +92,7 @@ describe('reverse exports', function () {
     expect(reversePackageExports(packageJson, './index-require.cjs')).toBe('my-addon');
     expect(reversePackageExports(packageJson, './index.js')).toBe('my-addon');
   });
+
   it('conditional subpath exports', function () {
     const packageJson = {
       name: 'my-addon',
@@ -112,6 +108,7 @@ describe('reverse exports', function () {
     expect(reversePackageExports(packageJson, './feature-node.cjs')).toBe('my-addon/feature.js');
     expect(reversePackageExports(packageJson, './feature.js')).toBe('my-addon/feature.js');
   });
+
   it('nested conditional exports', function () {
     const packageJson = {
       name: 'my-addon',
@@ -127,9 +124,26 @@ describe('reverse exports', function () {
     expect(reversePackageExports(packageJson, './feature-node.cjs')).toBe('my-addon');
     expect(reversePackageExports(packageJson, './feature.mjs')).toBe('my-addon');
   });
+
+  it('should throw when no exports entry is matching', function () {
+    const packageJson = {
+      name: 'my-addon',
+      exports: {
+        node: {
+          import: './feature-node.mjs',
+          require: './feature-node.cjs',
+        },
+        default: './feature.mjs',
+      },
+    };
+
+    expect(() => reversePackageExports(packageJson, './foo.bar')).toThrow(
+      'You tried to reverse exports for the file `./foo.bar` in package `my-addon` but it does not match any of the exports rules defined in package.json. This means it should not be possible to access directly.'
+    );
+  });
 });
 
-/* 8888888888888888888888888888 */
+///
 
 describe('_findKeyRecursively', function () {
   it('Returns "." when string is provided and matcher is matching', function () {