From 25e25cecdc54e30c60668cbd92077942e0949603 Mon Sep 17 00:00:00 2001 From: "Andrey Mikhaylov (lolmaus)" Date: Fri, 17 Nov 2023 18:47:28 +0300 Subject: [PATCH] reverse exports: address PR feedback --- packages/reverse-exports/src/index.ts | 14 +++--- .../tests/reverse-exports.test.ts | 50 ++++++++++++------- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/packages/reverse-exports/src/index.ts b/packages/reverse-exports/src/index.ts index 75a2cbc29..09ccc3ca6 100644 --- 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 index b9a056dfd..77ac17d20 100644 --- 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 () {