From d445aff580c86102c53ee0a56002c7fb94ec39e6 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 9 Aug 2024 18:25:43 -0400 Subject: [PATCH] Adjusting reverse-exports API I want to use `@embroider/reverse-exports` for a new case where it's not an error to discover that a file is internal. So I would rather that it didn't throw in this case. --- packages/core/src/module-resolver.ts | 22 ++++++-- packages/reverse-exports/src/index.ts | 28 ++++++---- .../tests/reverse-exports.test.ts | 56 +++++++++---------- 3 files changed, 60 insertions(+), 46 deletions(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index 5b5a85bd5..9c4a9846a 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -10,7 +10,7 @@ import type { Package, V2Package } from '@embroider/shared-internals'; import { explicitRelative, RewrittenPackageCache } from '@embroider/shared-internals'; import makeDebug from 'debug'; import assertNever from 'assert-never'; -import reversePackageExports from '@embroider/reverse-exports'; +import { externalName } from '@embroider/reverse-exports'; import { exports as resolveExports } from 'resolve.exports'; import { @@ -810,13 +810,19 @@ export class Resolver { `addon ${addon.name} declares app-js in its package.json with the illegal name "${inAddonName}". It must start with "./" to make it clear that it's relative to the addon` ); } + let specifier = externalName(addon.packageJSON, inAddonName); + if (!specifier) { + throw new Error( + `${addon.name}'s package.json app-js refers to ${inAddonName}, but that module is not accessible from outside the package` + ); + } let prevEntry = engineModules.get(inEngineName); switch (prevEntry?.type) { case undefined: engineModules.set(inEngineName, { type: 'app-only', 'app-js': { - specifier: reversePackageExports(addon.packageJSON, inAddonName), + specifier, fromFile: addonConfig.canResolveFromFile, fromPackageName: addon.name, }, @@ -830,7 +836,7 @@ export class Resolver { engineModules.set(inEngineName, { type: 'both', 'app-js': { - specifier: reversePackageExports(addon.packageJSON, inAddonName), + specifier, fromFile: addonConfig.canResolveFromFile, fromPackageName: addon.name, }, @@ -854,13 +860,19 @@ export class Resolver { `addon ${addon.name} declares fastboot-js in its package.json with the illegal name "${inAddonName}". It must start with "./" to make it clear that it's relative to the addon` ); } + let specifier = externalName(addon.packageJSON, inAddonName); + if (!specifier) { + throw new Error( + `${addon.name}'s package.json fastboot-js refers to ${inAddonName}, but that module is not accessible from outside the package` + ); + } let prevEntry = engineModules.get(inEngineName); switch (prevEntry?.type) { case undefined: engineModules.set(inEngineName, { type: 'fastboot-only', 'fastboot-js': { - specifier: reversePackageExports(addon.packageJSON, inAddonName), + specifier, fromFile: addonConfig.canResolveFromFile, fromPackageName: addon.name, }, @@ -874,7 +886,7 @@ export class Resolver { engineModules.set(inEngineName, { type: 'both', 'fastboot-js': { - specifier: reversePackageExports(addon.packageJSON, inAddonName), + specifier, fromFile: addonConfig.canResolveFromFile, fromPackageName: addon.name, }, diff --git a/packages/reverse-exports/src/index.ts b/packages/reverse-exports/src/index.ts index d4abfe695..6581185eb 100644 --- a/packages/reverse-exports/src/index.ts +++ b/packages/reverse-exports/src/index.ts @@ -1,6 +1,7 @@ import { posix } from 'path'; import { exports as resolveExports } from 'resolve.exports'; +type PkgJSON = { name: string; exports?: Exports }; type Exports = string | string[] | { [key: string]: Exports }; /** @@ -72,24 +73,27 @@ export function _findPathRecursively( throw new Error(`Unexpected type of obj: ${typeof exportsObj}`); } -export default function reversePackageExports( - { exports: exportsObj, name }: { exports?: Exports; name: string }, - relativePath: string -): string { - if (!exportsObj) { - return posix.join(name, relativePath); +/* + Takes a relativePath that is relative to the package root and produces its + externally-addressable name. + + Returns undefined for a relativePath that is forbidden to be accessed from the + outside. +*/ +export function externalName(pkg: PkgJSON, relativePath: string): string | undefined { + let { exports } = pkg; + if (!exports) { + return posix.join(pkg.name, relativePath); } - const maybeKeyValuePair = _findPathRecursively(exportsObj, candidate => { + const maybeKeyValuePair = _findPathRecursively(exports, candidate => { const regex = new RegExp(_prepareStringForRegex(candidate)); return regex.test(relativePath); }); if (!maybeKeyValuePair) { - 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.` - ); + return undefined; } const { key, value } = maybeKeyValuePair; @@ -98,7 +102,7 @@ export default function reversePackageExports( throw new Error('Expected value to be a string'); } - const maybeResolvedPaths = resolveExports({ name, exports: { [value]: key } }, relativePath); + const maybeResolvedPaths = resolveExports({ name: pkg.name, exports: { [value]: key } }, relativePath); if (!maybeResolvedPaths) { throw new Error( @@ -108,7 +112,7 @@ export default function reversePackageExports( const [resolvedPath] = maybeResolvedPaths; - return resolvedPath.replace(/^./, name); + return posix.join(pkg.name, resolvedPath); } export function _prepareStringForRegex(input: string): string { diff --git a/packages/reverse-exports/tests/reverse-exports.test.ts b/packages/reverse-exports/tests/reverse-exports.test.ts index ff94db842..f8c6d797b 100644 --- a/packages/reverse-exports/tests/reverse-exports.test.ts +++ b/packages/reverse-exports/tests/reverse-exports.test.ts @@ -1,14 +1,14 @@ -import reversePackageExports, { _findPathRecursively, _prepareStringForRegex } from '../src'; +import { externalName, _findPathRecursively, _prepareStringForRegex } from '../src'; describe('reverse exports', function () { it('exports is missing', function () { - expect(reversePackageExports({ name: 'best-addon' }, './dist/_app_/components/face.js')).toBe( + expect(externalName({ 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( + const actual = externalName( { name: 'my-addon', exports: './foo.js', @@ -19,7 +19,7 @@ describe('reverse exports', function () { }); it('exports is an object with one entry', function () { - const actual = reversePackageExports( + const actual = externalName( { name: 'my-addon', exports: { @@ -43,12 +43,12 @@ describe('reverse exports', function () { './glob/*': './grod/**/*.js', }, }; - expect(reversePackageExports(packageJson, './main.js')).toBe('my-addon'); - expect(reversePackageExports(packageJson, './secondary.js')).toBe('my-addon/sub/path'); - expect(reversePackageExports(packageJson, './directory/some/file.js')).toBe('my-addon/prefix/some/file.js'); - expect(reversePackageExports(packageJson, './other-directory/file.js')).toBe('my-addon/prefix/deep/file.js'); - 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'); + expect(externalName(packageJson, './main.js')).toBe('my-addon'); + expect(externalName(packageJson, './secondary.js')).toBe('my-addon/sub/path'); + expect(externalName(packageJson, './directory/some/file.js')).toBe('my-addon/prefix/some/file.js'); + expect(externalName(packageJson, './other-directory/file.js')).toBe('my-addon/prefix/deep/file.js'); + expect(externalName(packageJson, './yet-another/deep/file.js')).toBe('my-addon/other-prefix/deep/file'); + expect(externalName(packageJson, './grod/very/deep/file.js')).toBe('my-addon/glob/very/deep/file'); }); it('alternative exports', function () { @@ -58,8 +58,8 @@ describe('reverse exports', function () { './things/': ['./good-things/', './bad-things/'], }, }; - 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'); + expect(externalName(packageJson, './good-things/apple.js')).toBe('my-addon/things/apple.js'); + expect(externalName(packageJson, './bad-things/apple.js')).toBe('my-addon/things/apple.js'); }); it('conditional exports - simple abbreviated', function () { @@ -71,9 +71,9 @@ describe('reverse exports', function () { default: './index.js', }, }; - expect(reversePackageExports(packageJson, './index-module.js')).toBe('my-addon'); - expect(reversePackageExports(packageJson, './index-require.cjs')).toBe('my-addon'); - expect(reversePackageExports(packageJson, './index.js')).toBe('my-addon'); + expect(externalName(packageJson, './index-module.js')).toBe('my-addon'); + expect(externalName(packageJson, './index-require.cjs')).toBe('my-addon'); + expect(externalName(packageJson, './index.js')).toBe('my-addon'); }); it('conditional exports - simple non-abbreviated', function () { @@ -87,9 +87,9 @@ describe('reverse exports', function () { }, }, }; - expect(reversePackageExports(packageJson, './index-module.js')).toBe('my-addon'); - expect(reversePackageExports(packageJson, './index-require.cjs')).toBe('my-addon'); - expect(reversePackageExports(packageJson, './index.js')).toBe('my-addon'); + expect(externalName(packageJson, './index-module.js')).toBe('my-addon'); + expect(externalName(packageJson, './index-require.cjs')).toBe('my-addon'); + expect(externalName(packageJson, './index.js')).toBe('my-addon'); }); it('conditional subpath exports', function () { @@ -103,9 +103,9 @@ describe('reverse exports', function () { }, }, }; - expect(reversePackageExports(packageJson, './index.js')).toBe('my-addon'); - expect(reversePackageExports(packageJson, './feature-node.cjs')).toBe('my-addon/feature.js'); - expect(reversePackageExports(packageJson, './feature.js')).toBe('my-addon/feature.js'); + expect(externalName(packageJson, './index.js')).toBe('my-addon'); + expect(externalName(packageJson, './feature-node.cjs')).toBe('my-addon/feature.js'); + expect(externalName(packageJson, './feature.js')).toBe('my-addon/feature.js'); }); it('nested conditional exports', function () { @@ -119,12 +119,12 @@ describe('reverse exports', function () { default: './feature.mjs', }, }; - expect(reversePackageExports(packageJson, './feature-node.mjs')).toBe('my-addon'); - expect(reversePackageExports(packageJson, './feature-node.cjs')).toBe('my-addon'); - expect(reversePackageExports(packageJson, './feature.mjs')).toBe('my-addon'); + expect(externalName(packageJson, './feature-node.mjs')).toBe('my-addon'); + expect(externalName(packageJson, './feature-node.cjs')).toBe('my-addon'); + expect(externalName(packageJson, './feature.mjs')).toBe('my-addon'); }); - it('should throw when no exports entry is matching', function () { + it('should return undefined when no exports entry is matching', function () { const packageJson = { name: 'my-addon', exports: { @@ -136,9 +136,7 @@ describe('reverse exports', function () { }, }; - 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.' - ); + expect(externalName(packageJson, './foo.bar')).toBe(undefined); }); it('conditional exports: using a single asterisk as glob for nested path', function () { @@ -154,7 +152,7 @@ describe('reverse exports', function () { }, }; - expect(reversePackageExports(packageJson, './dist/_app_/components/welcome-page.js')).toBe( + expect(externalName(packageJson, './dist/_app_/components/welcome-page.js')).toBe( 'my-v2-addon/_app_/components/welcome-page' ); });