Skip to content

Commit 088f761

Browse files
os-litantclaude
andauthored
fix(types): recognise an aliased package root by the name its declaration promises (#15563)
`createHostImporter`'s #13330 condition re-decision recognised the package root by walking up from the CJS resolver's answer until it found a manifest named after the DECLARATION KEY. An aliased install — `{"foo": "npm:bar@1"}` — puts a manifest named `bar` there, so the walk never matched, `esmEntryForDeclared` returned `undefined`, and `?? resolved` handed back the entry CommonJS resolution had answered: the `require` condition. For an aliased dual publish that is the two-instances-of-one-package split #13330 exists to remove. The expectation now comes from `declaredManifestName` — the reading #14278 gave the ESM-only fallback finder — so both legs expect one name from one place. The helper moves above both rather than staying inside the fallback section. Only the EXPECTATION moves, never the strictness: an alias naming one package still does not license a directory holding another, and a non-aliased declaration is still verified against its key. The exports SUBPATH stays cut from the KEY, which is what the specifier is spelled with. `packageRootOf` is module-private, called at one site, and named by neither subpath export of `@objectstack/types`, so the signature change is unreachable by any consumer. Part of #15044 Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N Co-authored-by: Claude <noreply@anthropic.com>
1 parent e39619d commit 088f761

3 files changed

Lines changed: 430 additions & 60 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/types": patch
3+
---
4+
5+
`createHostImporter` now loads the `import` build of an ALIASED dual-published package, instead of silently keeping its `require` build.
6+
7+
An alias declaration — `{"dependencies": {"foo": "npm:bar@1"}}` — installs a package whose manifest is named `bar` under the key `foo`. On the path where CommonJS resolution SUCCEEDS, the importer re-decides only the CONDITION (it asks the package which entry an `import()` gets, so the caller's ESM chain and this load share one instance). That re-decision recognised the package root by walking up from the resolved entry until it found a manifest named after the DECLARATION KEY — `foo` — while an aliased install's manifest is named `bar`. The walk therefore never matched, the re-decision produced nothing, and the load fell back to whatever the CommonJS resolver had answered: the `require` condition.
8+
9+
For an aliased dual publish that left the process holding two live copies of one package — the CommonJS build behind the host importer, the `import` build in the caller's own chain — which is exactly the split the condition re-decision exists to remove: a plugin registry, a singleton kernel, a module-level cache, one copy each.
10+
11+
The expectation now comes from the host's own declaration (`npm:name@range`, aliased `workspace:name@range`), the same reading the ESM-only fallback finder has used since it learned about aliases. Nothing about the check's strictness moves: an alias naming one package still does not license a directory holding another, and a non-aliased declaration is still verified against its key. Declarations that name a LOCATION rather than a package (`link:`, `file:`) carry no name to expect, so they keep today's behaviour unchanged.
12+
13+
Measured population for the behaviour change: zero aliased declarations exist across this workspace's 875 dependency declarations, and 867 of 867 installed declarations already match their key — no ordinary, non-aliased install reaches this path.

packages/types/src/node.test.ts

Lines changed: 315 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1514,3 +1514,318 @@ describe('an aliased install is verified against the name its DECLARATION names
15141514
expect(hostImportFailureKind(err)).toBe('declared-unresolvable');
15151515
});
15161516
});
1517+
1518+
/**
1519+
* ── #15044: the SUCCEEDING leg recognised the package by the DECLARATION KEY ──
1520+
*
1521+
* #14278 taught the #14041 FALLBACK finder that `{"foo": "npm:bar@1"}` installs
1522+
* a package named `bar`. The same blindness survived one leg over, on the path
1523+
* where the CJS resolve SUCCEEDS — #13330's condition re-decision:
1524+
* `packageRootOf` walked up from the resolved entry looking for a manifest
1525+
* named `foo`, and an aliased install's manifest is named `bar`. The walk
1526+
* therefore never matched, `esmEntryForDeclared` returned `undefined`, and
1527+
* `?? resolved` handed back the entry the CJS resolver had answered — the
1528+
* `require` condition.
1529+
*
1530+
* ⇒ For an aliased DUAL-PUBLISHED package the host importer silently kept
1531+
* pre-#13330 behaviour: the CommonJS build loads while the caller's own ESM
1532+
* chain loads the `import` build. That is the two-instances-of-one-package
1533+
* split #13330 exists to remove, so what these cases assert is the SHARED
1534+
* INSTANCE, not the file name — a test that only checked which path was
1535+
* imported would pass on a fix that loaded the right file into the wrong
1536+
* instance.
1537+
*
1538+
* This leg is NOT #14278's: that one fired only inside `hostRequire.resolve`'s
1539+
* catch, a hard failure before #14041, so it could not move a working load.
1540+
* This one moves a load that SUCCEEDS today, which is why the non-aliased
1541+
* controls below matter more than another aliased assertion — the risk in the
1542+
* change is a regression in the ordinary case, not the aliased case it fixes.
1543+
*/
1544+
describe('an aliased dual-published package loads its `import` build too (#15044)', () => {
1545+
/** Keys the HOST writes; none of them is the name of the package installed there. */
1546+
const REGISTRY_KEY = 'aliased-registry';
1547+
const DRIVER_KEY = 'aliased-driver';
1548+
const SUBPATH_KEY = 'aliased-subpaths';
1549+
/** Manifest names the ALIAS values promise. */
1550+
const REGISTRY_NAME = '@fixture/alias-dual-registry';
1551+
const DRIVER_NAME = '@fixture/alias-dual-driver';
1552+
const SUBPATH_NAME = '@fixture/alias-dual-subpaths';
1553+
/** An ordinary, NON-aliased dual publish living in the same app. */
1554+
const PLAIN = '@fixture/plain-dual';
1555+
1556+
/** Module-scope state, published as both builds — the `tsup` dual-build shape. */
1557+
const REGISTRY_ESM = `export const BUILD = 'esm';
1558+
const registered = [];
1559+
export function register(name) { registered.push(name); }
1560+
export function listRegistered() { return [...registered]; }
1561+
`;
1562+
const REGISTRY_CJS = `const registered = [];
1563+
exports.BUILD = 'cjs';
1564+
exports.register = (name) => { registered.push(name); };
1565+
exports.listRegistered = () => [...registered];
1566+
`;
1567+
/**
1568+
* A driver package whose entire contract is the load-time side effect. It
1569+
* imports the registry by the ALIAS KEY, which is the only spelling an
1570+
* aliased install makes importable — the alias target's own name resolves
1571+
* nowhere.
1572+
*/
1573+
const DRIVER_ESM = `import { register } from '${REGISTRY_KEY}';
1574+
register('probe');
1575+
export const BUILD = 'esm';
1576+
`;
1577+
const DRIVER_CJS = `const { register } = require('${REGISTRY_KEY}');
1578+
register('probe');
1579+
exports.BUILD = 'cjs';
1580+
`;
1581+
1582+
/** Nested, `types` first — the map `tsup` emits, so the walk cannot match a flat string. */
1583+
const DUAL: unknown = {
1584+
'.': {
1585+
import: { types: './dist/index.d.ts', default: './dist/index.js' },
1586+
require: { types: './dist/index.d.cts', default: './dist/index.cjs' },
1587+
},
1588+
};
1589+
1590+
const roots: string[] = [];
1591+
1592+
afterAll(() => {
1593+
for (const dir of roots) rmSync(dir, { recursive: true, force: true });
1594+
});
1595+
1596+
/**
1597+
* Install a package NAMED `manifestName` at `node_modules/<key>` — the
1598+
* on-disk shape every aliasing package manager produces.
1599+
*/
1600+
function installAs(
1601+
root: string,
1602+
key: string,
1603+
manifestName: string,
1604+
exportsField: unknown,
1605+
files: Record<string, string>,
1606+
): void {
1607+
const dir = join(root, 'node_modules', ...key.split('/'));
1608+
mkdirSync(dir, { recursive: true });
1609+
writeFileSync(
1610+
join(dir, 'package.json'),
1611+
JSON.stringify({
1612+
name: manifestName,
1613+
version: '0.0.0-fixture',
1614+
type: 'module',
1615+
main: 'dist/index.cjs',
1616+
exports: exportsField,
1617+
}),
1618+
'utf8',
1619+
);
1620+
for (const rel of Object.keys(files)) {
1621+
const target = join(dir, rel);
1622+
mkdirSync(dirname(target), { recursive: true });
1623+
writeFileSync(target, files[rel] as string, 'utf8');
1624+
}
1625+
}
1626+
1627+
/**
1628+
* A fresh app per case. The ESM module cache is keyed by absolute URL, so a
1629+
* shared fixture directory would let one case's load answer the next one's
1630+
* question — the failure mode this whole suite exists to detect.
1631+
*/
1632+
function app(tag: string, extraDependencies: Record<string, string> = {}): string {
1633+
const root = mkdtempSync(join(tmpdir(), `os-aliased-dual-${tag}-`));
1634+
roots.push(root);
1635+
writeFileSync(
1636+
join(root, 'package.json'),
1637+
JSON.stringify({
1638+
name: 'aliased-dual-host-fixture',
1639+
type: 'module',
1640+
dependencies: {
1641+
[REGISTRY_KEY]: `npm:${REGISTRY_NAME}@1`,
1642+
[DRIVER_KEY]: `npm:${DRIVER_NAME}@1`,
1643+
[SUBPATH_KEY]: `npm:${SUBPATH_NAME}@^2.0.0`,
1644+
[PLAIN]: '^1.0.0',
1645+
...extraDependencies,
1646+
},
1647+
}),
1648+
'utf8',
1649+
);
1650+
installAs(root, REGISTRY_KEY, REGISTRY_NAME, DUAL, {
1651+
'dist/index.js': REGISTRY_ESM,
1652+
'dist/index.cjs': REGISTRY_CJS,
1653+
});
1654+
installAs(root, DRIVER_KEY, DRIVER_NAME, DUAL, {
1655+
'dist/index.js': DRIVER_ESM,
1656+
'dist/index.cjs': DRIVER_CJS,
1657+
});
1658+
installAs(
1659+
root,
1660+
SUBPATH_KEY,
1661+
SUBPATH_NAME,
1662+
{
1663+
'.': { import: './dist/index.js', require: './dist/index.cjs' },
1664+
'./plugin': { import: './dist/plugin.js', require: './dist/plugin.cjs' },
1665+
},
1666+
{
1667+
'dist/index.js': "export const WHERE = 'root-esm';\n",
1668+
'dist/index.cjs': "exports.WHERE = 'root-cjs';\n",
1669+
'dist/plugin.js': "export const WHERE = 'plugin-esm';\n",
1670+
'dist/plugin.cjs': "exports.WHERE = 'plugin-cjs';\n",
1671+
},
1672+
);
1673+
// NON-aliased, installed under its own name: the ordinary case, in the very
1674+
// same app, so a change that moved it would redden here.
1675+
installAs(root, PLAIN, PLAIN, DUAL, {
1676+
'dist/index.js': "export const BUILD = 'plain-esm';\n",
1677+
'dist/index.cjs': "exports.BUILD = 'plain-cjs';\n",
1678+
});
1679+
return root;
1680+
}
1681+
1682+
/** The instance an ESM consumer chain holds — what the Runtime reads. */
1683+
function esmInstance(root: string): Promise<{ BUILD: string; listRegistered: () => string[] }> {
1684+
return import(pathToFileURL(join(root, 'node_modules', REGISTRY_KEY, 'dist', 'index.js')).href);
1685+
}
1686+
1687+
/** The instance a CommonJS load lands in — where the registration used to go. */
1688+
function cjsInstance(root: string): Promise<{ BUILD: string; listRegistered: () => string[] }> {
1689+
return import(pathToFileURL(join(root, 'node_modules', REGISTRY_KEY, 'dist', 'index.cjs')).href);
1690+
}
1691+
1692+
// No `fallbackImport`: every fixture here is DECLARED, so the undeclared leg
1693+
// (the only consumer of that base) is never reached.
1694+
const importer = (root: string) => createHostImporter(root);
1695+
1696+
it('CONTROL: the reader can see BOTH answers, so an empty registry is a reading', async () => {
1697+
// Every assertion below rests on `listRegistered()` being able to come back
1698+
// non-empty. A probe that could only ever return `[]` would make the whole
1699+
// describe pass on a broken fix — so it is proved here, on the same
1700+
// instrument, before anything is measured with it.
1701+
const root = app('control');
1702+
const esm = await esmInstance(root);
1703+
const cjs = await cjsInstance(root);
1704+
1705+
expect(esm.BUILD).toBe('esm');
1706+
expect(cjs.BUILD).toBe('cjs');
1707+
expect(esm.listRegistered()).toEqual([]);
1708+
expect(cjs.listRegistered()).toEqual([]);
1709+
});
1710+
1711+
it('PRECONDITION: the CJS resolve SUCCEEDS here — this is #13330\'s leg, not #14041\'s', () => {
1712+
// What separates this card from #14278: nothing throws, so the fallback
1713+
// finder never runs and the load being corrected is one that works today.
1714+
// If a future Node stopped answering the `require` condition here, this
1715+
// says so rather than leaving the fix looking like a no-op.
1716+
const root = app('precondition');
1717+
expect(createHostRequire(root).resolve(DRIVER_KEY)).toMatch(/dist[/\\]index\.cjs$/);
1718+
});
1719+
1720+
it('THE CARD: an aliased dual publish loads the `import` build, not the `require` one', async () => {
1721+
const root = app('import-condition');
1722+
expect((await importer(root)(DRIVER_KEY)).BUILD).toBe('esm');
1723+
});
1724+
1725+
it("a driver's load-time registration lands in the instance an ESM caller reads", async () => {
1726+
// The defect stated as its consequence — the two-instances split. Before
1727+
// the fix this stayed `[]`.
1728+
const root = app('visible');
1729+
expect((await esmInstance(root)).listRegistered()).toEqual([]);
1730+
await importer(root)(DRIVER_KEY);
1731+
expect((await esmInstance(root)).listRegistered()).toEqual(['probe']);
1732+
});
1733+
1734+
it('and no longer lands in the CommonJS instance nothing reads', async () => {
1735+
// The other direction: the registration MOVED, it was not duplicated.
1736+
const root = app('cjs-empty');
1737+
await importer(root)(DRIVER_KEY);
1738+
expect((await cjsInstance(root)).listRegistered()).toEqual([]);
1739+
expect((await esmInstance(root)).listRegistered()).toEqual(['probe']);
1740+
});
1741+
1742+
it('the exports SUBPATH is still cut from the KEY, not from the aliased name', async () => {
1743+
// The two names are different questions (see `esmEntryForDeclared`). The
1744+
// package ROOT is recognised by the name the declaration promises; the
1745+
// SUBPATH is what the specifier spells after the key. Using the aliased
1746+
// name for both would compute a nonsense subpath here.
1747+
const root = app('subpath');
1748+
expect((await importer(root)(SUBPATH_KEY)).WHERE).toBe('root-esm');
1749+
expect((await importer(root)(`${SUBPATH_KEY}/plugin`)).WHERE).toBe('plugin-esm');
1750+
});
1751+
1752+
it('a `workspace:` ALIAS is recognised too; a plain `workspace:` range is a RANGE', async () => {
1753+
const aliased = app('workspace-alias', { 'ws-dual': 'workspace:@fixture/ws-dual-target@*' });
1754+
installAs(aliased, 'ws-dual', '@fixture/ws-dual-target', DUAL, {
1755+
'dist/index.js': "export const BUILD = 'ws-esm';\n",
1756+
'dist/index.cjs': "exports.BUILD = 'ws-cjs';\n",
1757+
});
1758+
expect((await importer(aliased)('ws-dual')).BUILD).toBe('ws-esm');
1759+
1760+
// `workspace:*` carries no name, so the key stays the expectation and the
1761+
// package installed under its own name is recognised exactly as before.
1762+
const plain = app('workspace-range', { '@fixture/ws-plain-dual': 'workspace:*' });
1763+
installAs(plain, '@fixture/ws-plain-dual', '@fixture/ws-plain-dual', DUAL, {
1764+
'dist/index.js': "export const BUILD = 'ws-plain-esm';\n",
1765+
'dist/index.cjs': "exports.BUILD = 'ws-plain-cjs';\n",
1766+
});
1767+
expect((await importer(plain)('@fixture/ws-plain-dual')).BUILD).toBe('ws-plain-esm');
1768+
});
1769+
1770+
it('CONTROL (non-aliased): an ordinary install\'s selected entry does not move', async () => {
1771+
// The control worth more than another aliased assertion: the risk in this
1772+
// change is a regression in the ordinary case, which already works. Same
1773+
// app, same importer, plain range, installed under its own name.
1774+
const root = app('plain-unmoved');
1775+
expect((await importer(root)(PLAIN)).BUILD).toBe('plain-esm');
1776+
});
1777+
1778+
it('CONTROL (non-aliased): a package publishing only `require` still loads', async () => {
1779+
// Narrowness, on the ordinary path: there is no `import` entry to prefer,
1780+
// so the resolved CJS path is used exactly as before. The fix may not turn
1781+
// a working load into a failure.
1782+
const root = app('require-only', { '@fixture/plain-require-only': '^1.0.0' });
1783+
installAs(root, '@fixture/plain-require-only', '@fixture/plain-require-only', {
1784+
'.': { require: './dist/index.cjs' },
1785+
}, { 'dist/index.cjs': "exports.BUILD = 'require-only';\n" });
1786+
expect((await importer(root)('@fixture/plain-require-only')).BUILD).toBe('require-only');
1787+
});
1788+
1789+
it('TIGHTNESS: an alias naming one package does not license a directory holding another', async () => {
1790+
// The expectation moved; the comparison did not. The declaration says this
1791+
// directory holds `@fixture/alias-declared`; it holds something else, so
1792+
// the walk must NOT recognise it as the declared package's root and the
1793+
// load stays on the CJS resolver's own answer — today's behaviour for an
1794+
// unrecognised root, unchanged.
1795+
const root = app('alias-mismatch', { 'aliased-wrong': 'npm:@fixture/alias-declared@1' });
1796+
installAs(root, 'aliased-wrong', '@fixture/alias-installed', DUAL, {
1797+
'dist/index.js': "export const BUILD = 'imposter-esm';\n",
1798+
'dist/index.cjs': "exports.BUILD = 'imposter-cjs';\n",
1799+
});
1800+
expect((await importer(root)('aliased-wrong')).BUILD).toBe('imposter-cjs');
1801+
});
1802+
1803+
it('TIGHTNESS: a NON-aliased declaration is still verified against the KEY', async () => {
1804+
// The other half of the same property: a plain range declares no alias, so
1805+
// a directory holding a different package is still not recognised. An
1806+
// aliased-install green that also greened this one would mean the walk got
1807+
// looser, not smarter.
1808+
const root = app('plain-mismatch', { '@fixture/plain-range': '^1.0.0' });
1809+
installAs(root, '@fixture/plain-range', '@fixture/somebody-else', DUAL, {
1810+
'dist/index.js': "export const BUILD = 'somebody-else-esm';\n",
1811+
'dist/index.cjs': "exports.BUILD = 'somebody-else-cjs';\n",
1812+
});
1813+
expect((await importer(root)('@fixture/plain-range')).BUILD).toBe('somebody-else-cjs');
1814+
});
1815+
1816+
it('BOUNDARY: a `link:` target whose manifest names something else keeps today\'s load', async () => {
1817+
// Declared residue, pinned rather than left silent. `link:` / `file:` name
1818+
// a LOCATION, so no package name can be parsed out of them and the key
1819+
// stays the expectation — exactly as #14278 ruled for the sibling leg. On
1820+
// THIS leg the consequence is a load rather than a refusal: the `require`
1821+
// build keeps loading. Widening it would mean recognising a package root
1822+
// the host never named, which is the direction the manifest-name check
1823+
// exists to refuse.
1824+
const root = app('link-mismatch', { 'linked-other': 'link:../elsewhere' });
1825+
installAs(root, 'linked-other', '@fixture/some-other-name', DUAL, {
1826+
'dist/index.js': "export const BUILD = 'linked-esm';\n",
1827+
'dist/index.cjs': "exports.BUILD = 'linked-cjs';\n",
1828+
});
1829+
expect((await importer(root)('linked-other')).BUILD).toBe('linked-cjs');
1830+
});
1831+
});

0 commit comments

Comments
 (0)