diff --git a/create-hook.mjs b/create-hook.mjs index 0471800..77678b0 100644 --- a/create-hook.mjs +++ b/create-hook.mjs @@ -198,21 +198,31 @@ function emitWarning (err) { * of how the loader is driven, so both the synchronous and asynchronous paths * share it. * - * The value is read from `namespaceVar`, the wrapper's namespace binding for the - * module that *defines* the export. For a module's own exports that is the - * wrapped module itself; for a name re-exported through `export *` it is the - * leaf that declares it. Reading from the defining module rather than the - * aggregating one keeps the value resolvable when the same binding reaches the - * aggregator through more than one re-export chain — Node sees those chains as - * distinct wrapper modules and leaves the name ambiguous (hence `undefined`) on - * the aggregate namespace, while the defining module always holds it (#171). + * The value is read from `namespaceVar`, the wrapped module's own namespace. A + * name pulled in through `export *` additionally passes `fallbackVar`, the + * namespace of the module that *defines* it: the binder reads the aggregate + * namespace first (so a hook applied along the re-export chain is still + * observed) and falls back to the defining module when the aggregate does not + * hold the value. That fallback covers two cases the aggregate namespace cannot + * serve synchronously: + * - the same binding reaching the aggregator through more than one re-export + * chain, which Node leaves ambiguous (hence `undefined`) on the aggregate + * namespace because under iitm the chains are distinct wrapper modules, + * while the defining module always holds it (#171); and + * - a circular `export *` where the defining module is still in its temporal + * dead zone on the aggregate namespace when this wrapper evaluates, yet has + * already hoisted the binding in its own namespace, so deferring to a late + * async retry would let a synchronous importer read `undefined` (#269). * * @param {string} n The exported name. * @param {string} srcUrl The URL of the module the export belongs to. - * @param {string} namespaceVar The wrapper binding holding `srcUrl`'s namespace. + * @param {string} namespaceVar The wrapper binding holding the primary namespace. + * @param {string} [fallbackVar] The wrapper binding holding the defining + * module's namespace, for `export *`-sourced names. Omitted for a module's own + * exports, which only ever read from `namespaceVar`. * @returns {string} */ -function buildSetter (n, srcUrl, namespaceVar) { +function buildSetter (n, srcUrl, namespaceVar, fallbackVar) { const variableName = `$${n.replace(/[^a-zA-Z0-9_$]/g, '_')}` const objectKey = JSON.stringify(n) const reExportedName = n === 'default' ? n : objectKey @@ -226,8 +236,10 @@ function buildSetter (n, srcUrl, namespaceVar) { ? '' : `export { ${variableName} as ${reExportedName} }` + const fallbackArg = fallbackVar === undefined ? '' : `, ${fallbackVar}` + return `let ${variableName} -__binder.bind(${objectKey}, ${namespaceVar}, v => { ${variableName} = v }, () => ${variableName}, ${useFallback}) +__binder.bind(${objectKey}, ${namespaceVar}, v => { ${variableName} = v }, () => ${variableName}, ${useFallback}${fallbackArg}) ${reExportLine}` } @@ -254,10 +266,10 @@ ${reExportLine}` * before descending into its subtree and removed once that subtree finishes, so * it tracks the active path rather than every URL ever visited. * @param {Map} [params.originNamespaces] Shared registry mapping - * a defining-module URL to the wrapper namespace alias a same-origin `export *` - * collision must read it from. Absent until the first such collision; then - * threaded through the recursion so one defining module yields one alias and - * {@link buildWrapperSource} imports each once. Only `*`-collided names use it; + * a defining-module URL to the wrapper namespace alias its `export *`-sourced + * names fall back to. Absent until the first `export *`; then threaded through + * the recursion so one defining module yields one alias and + * {@link buildWrapperSource} imports each once. Only `*`-sourced names use it; * every other export reads from the wrapped module's own `namespace`. * * @returns {Generator, origins: (Map | undefined), originNamespaces: (Map | undefined) }>} @@ -265,7 +277,7 @@ ${reExportLine}` * setters for all the exports from the module and any transitive export all * modules. `origins` (the defining module per `*`-sourced name) is `undefined` * for a module with no `export *`; `originNamespaces` stays `undefined` unless a - * same-origin `*` collision actually needed an alias. + * `*` re-export actually minted an alias. */ function * processModule ({ srcUrl, context, excludeDefault = false, depth = 0, seen, originNamespaces }) { const exportNames = yield * getExports(srcUrl, context) @@ -279,15 +291,13 @@ function * processModule ({ srcUrl, context, excludeDefault = false, depth = 0, // and one write per name, not two. let starOrigins - // A name pulled in through more than one `export *` chain that all bottom out - // at the same module stays exported (ECMAScript ResolveExport; - // tc39/ecma262#3715), but the *aggregate* namespace this wrapper imports drops - // it: under iitm the chains are distinct wrapped modules, so Node sees the - // re-export as ambiguous and the name reads back undefined. Only those names - // must instead read from their defining module's own namespace, which always - // holds the value. `originNamespaces` maps such a defining module to the alias - // the wrapper imports for it; it is allocated on the first surviving - // collision, so a module without one emits no extra import (#171). + // Every `export *`-sourced name falls back to its defining module's own + // namespace when the aggregate namespace this wrapper imports doesn't hold the + // value — because that binding is ambiguous across chains (#171) or still in + // its temporal dead zone on the aggregate mid-cycle (#269). `originNamespaces` + // maps each defining module to the alias the wrapper imports for it; it is + // allocated on the first `export *`, so a module without one emits no extra + // import. const ensureOriginNamespace = (origin) => { originNamespaces ??= new Map() let alias = originNamespaces.get(origin) @@ -300,27 +310,26 @@ function * processModule ({ srcUrl, context, excludeDefault = false, depth = 0, const addSetter = (name, setter, isStarExport, origin) => { if (setters.has(name)) { - if (isStarExport) { - // `starOrigins.has(name)` means the existing entry also came from a `*` - // re-export (an explicit export would not be tracked here). - if (starOrigins.has(name)) { - if (starOrigins.get(name) === origin) { - // The same binding reached through two `*` re-export chains. It - // stays exported, but the aggregate namespace dropped it, so point - // its setter at the defining module's namespace instead. - setters.set(name, buildSetter(name, origin, ensureOriginNamespace(origin))) - } else { - // Genuinely ambiguous: two `*` re-exports name it from different - // modules. Per ResolveExport the name is excluded entirely. - setters.delete(name) - starOrigins.delete(name) - } + // `starOrigins.has(name)` means the existing entry also came from a `*` + // re-export (an explicit export would not be tracked here). + if (isStarExport && starOrigins.has(name)) { + if (starOrigins.get(name) !== origin) { + // Genuinely ambiguous: two `*` re-exports name it from different + // modules. Per ResolveExport the name is excluded entirely. + setters.delete(name) + starOrigins.delete(name) } - // An explicit export already shadows the `*` re-export; leave it. + // Same binding reached through two `*` chains: it stays exported and the + // existing setter already falls back to that defining module, so keep it. } + // An explicit export already shadows the `*` re-export; leave it. } else { if (isStarExport) { starOrigins.set(name, origin) + // Read from the aggregate namespace, falling back to the defining + // module (see ensureOriginNamespace) so the value resolves even when the + // aggregate drops it or holds it in a temporal dead zone. + setter = buildSetter(name, origin, 'namespace', ensureOriginNamespace(origin)) } setters.set(name, setter) @@ -384,9 +393,9 @@ function * processModule ({ srcUrl, context, excludeDefault = false, depth = 0, // the whole tree) rather than orphaning the child's into a second Map. originNamespaces ??= sub.originNamespaces - // Star targets build their setters against `namespace` like any other - // module; only a surviving same-origin collision (in addSetter) rewrites - // the affected name to read from its defining module's alias. + // addSetter rebuilds each `*`-sourced name to read from the aggregate + // namespace with a fallback to its defining module, so the sub's own + // setter string is only a placeholder here. for (const [name, setter] of sub.setters) { addSetter(name, setter, true, sub.origins?.get(name) ?? result.url) } @@ -650,11 +659,10 @@ export function createHook (meta) { // iitm's proxy. Pure string generation shared by the asynchronous and // synchronous `load` paths. function buildWrapperSource (realUrl, setters, originalSpecifier, originNamespaces) { - // The wrapped module imports its namespace as `namespace`, which serves - // every export but the ones a same-origin `export *` collision forced onto - // their defining module (#171): the aggregate namespace drops those as - // ambiguous under iitm, so each such defining module gets its own alias the - // wrapper imports. Absent the registry (no such collision) nothing is added. + // The wrapped module imports its namespace as `namespace`. Every + // `export *`-sourced name additionally falls back to its defining module, + // so each such module gets its own alias the wrapper imports here (#171, + // #269). Absent the registry (the module has no `export *`) nothing is added. let originImports = '' if (originNamespaces !== undefined) { for (const [originUrl, alias] of originNamespaces) { diff --git a/lib/register.js b/lib/register.js index 77cd642..427a48d 100644 --- a/lib/register.js +++ b/lib/register.js @@ -91,12 +91,27 @@ class ModuleBinder { * @param {() => unknown} read Reads the wrapper's local binding. * @param {boolean} useFallback Fall back to `source.default` (the synthetic * `module.exports` name a builtin does not expose on its ESM namespace). + * @param {object} [fallbackSource] The defining module's namespace for a name + * re-exported through `export *`. Read when `source` yields `undefined` or is + * in its temporal dead zone, so an ambiguous (#171) or mid-cycle (#269) + * binding still resolves synchronously instead of only on a later retry. * @returns {void} */ - bind (key, source, write, read, useFallback) { - const readSource = useFallback + bind (key, source, write, read, useFallback, fallbackSource) { + const readPrimary = useFallback ? () => source[key] ?? source.default : () => source[key] + const readSource = fallbackSource === undefined + ? readPrimary + : () => { + try { + const value = readPrimary() + if (value !== undefined) return value + } catch (error) { + if (!(error instanceof ReferenceError)) throw error + } + return fallbackSource[key] + } this.#overridden[key] = false let deferred = false try { diff --git a/test/fixtures/star-reexport-tdz-barrel.mjs b/test/fixtures/star-reexport-tdz-barrel.mjs new file mode 100644 index 0000000..07a0b35 --- /dev/null +++ b/test/fixtures/star-reexport-tdz-barrel.mjs @@ -0,0 +1,4 @@ +// A barrel that re-exports the leaf's bindings with `export *`. This is the +// module whose wrapper snapshots `Foo`/`IsFoo` while they are still in the +// leaf's temporal dead zone (see star-reexport-tdz-leaf.mjs). +export * from './star-reexport-tdz-leaf.mjs' diff --git a/test/fixtures/star-reexport-tdz-enter.mjs b/test/fixtures/star-reexport-tdz-enter.mjs new file mode 100644 index 0000000..52ea8d8 --- /dev/null +++ b/test/fixtures/star-reexport-tdz-enter.mjs @@ -0,0 +1,9 @@ +// Imported by the top module *before* the barrel, so the leaf is entered (and +// left on the evaluation stack mid-cycle) ahead of the barrel. This is what +// forces the barrel to evaluate before the leaf finishes, reproducing the +// temporal-dead-zone ordering from typebox (issue #269). +import { Foo } from './star-reexport-tdz-leaf.mjs' + +export function useFoo (value) { + return Foo(value) +} diff --git a/test/fixtures/star-reexport-tdz-leaf.mjs b/test/fixtures/star-reexport-tdz-leaf.mjs new file mode 100644 index 0000000..c829f99 --- /dev/null +++ b/test/fixtures/star-reexport-tdz-leaf.mjs @@ -0,0 +1,21 @@ +// The leaf that *defines* the exports. It pulls in the barrel that re-exports +// it (`import * as`), which is what closes the circular `export *` loop: +// barrel -> leaf -> barrel. Because the barrel is reached again from here, it +// ends up deeper in the same strongly-connected component than this leaf and +// therefore evaluates *before* it, so the leaf's bindings are still in their +// temporal dead zone when the barrel's wrapper snapshots them (issue #269). +import * as barrel from './star-reexport-tdz-barrel.mjs' + +export function Foo (value) { + return value +} + +// Reads the barrel namespace back so the circular dependency is genuine (once +// settled, `barrel.Foo` is this module's own `Foo`). +export function sameAsBarrel () { + return barrel.Foo === Foo +} + +export function IsFoo (value) { + return typeof value === 'object' +} diff --git a/test/fixtures/star-reexport-tdz-top.mjs b/test/fixtures/star-reexport-tdz-top.mjs new file mode 100644 index 0000000..59a9aa6 --- /dev/null +++ b/test/fixtures/star-reexport-tdz-top.mjs @@ -0,0 +1,6 @@ +// The entry aggregator. `enter` comes first so the leaf is entered before the +// barrel; the barrel then contributes `Foo`/`IsFoo` through its `export *`. +// Mirrors typebox's `type/index.js`, which `export *`s several barrels where a +// deeply cross-imported leaf (`template_literal`) surfaces as `undefined`. +export * from './star-reexport-tdz-enter.mjs' +export * from './star-reexport-tdz-barrel.mjs' diff --git a/test/hook/star-reexport-tdz.mjs b/test/hook/star-reexport-tdz.mjs new file mode 100644 index 0000000..257df0e --- /dev/null +++ b/test/hook/star-reexport-tdz.mjs @@ -0,0 +1,19 @@ +import * as top from '../fixtures/star-reexport-tdz-top.mjs' +import { strictEqual } from 'assert' +import Hook from '../../index.js' + +// Regression test for issue #269 (typebox's exports coming back `undefined` +// under the loader). +// +// `Foo`/`IsFoo` are function declarations defined in the leaf module and +// re-exported through a barrel with `export *`. A circular `export *` loop +// (barrel -> leaf -> barrel) makes the barrel's wrapper evaluate while the leaf +// is still in its temporal dead zone, so the wrapper snapshots the bindings as +// `undefined`. Without the loader these are always functions (hoisted +// declarations, live bindings), so the loader must expose them the same way and +// not lose them to a deferred read that only recovers on a later async retry. +Hook(() => {}) + +strictEqual(typeof top.Foo, 'function') +strictEqual(typeof top.IsFoo, 'function') +strictEqual(typeof top.useFoo, 'function') diff --git a/test/low-level/module-binder.mjs b/test/low-level/module-binder.mjs index d96c912..9bf78c3 100644 --- a/test/low-level/module-binder.mjs +++ b/test/low-level/module-binder.mjs @@ -119,3 +119,71 @@ function makeSlot (initial) { const { write, read } = makeSlot(undefined) throws(() => binder.bind('foo', source, write, read, false), TypeError) } + +// A fallbackSource is not consulted while the primary source holds a value. +{ + const binder = new ModuleBinder() + const source = { foo: 1 } + const fallback = { get foo () { throw new Error('must not be read') } } + const { slot, write, read } = makeSlot(undefined) + binder.bind('foo', source, write, read, false, fallback) + strictEqual(slot.value, 1, 'primary value wins over the fallback') +} + +// A primary source without the value (an `export *` name the aggregate +// namespace dropped as ambiguous) reads the fallbackSource, synchronously. +{ + const binder = new ModuleBinder() + const source = {} + const fallback = { foo: 2 } + const { slot, write, read } = makeSlot(undefined) + binder.bind('foo', source, write, read, false, fallback) + strictEqual(slot.value, 2, 'undefined primary resolved from the fallback at bind time') +} + +// A primary source still in its dead zone reads the fallbackSource, +// synchronously — not deferred to a retry. +{ + const binder = new ModuleBinder() + const source = { get foo () { throw new ReferenceError('tdz') } } + const fallback = { foo: 3 } + const { slot, write, read } = makeSlot(undefined) + binder.bind('foo', source, write, read, false, fallback) + strictEqual(slot.value, 3, 'TDZ primary resolved from the fallback at bind time') +} + +// A non-ReferenceError from the primary still propagates; the fallback is no +// license to swallow real errors. +{ + const binder = new ModuleBinder() + const source = { get foo () { throw new TypeError('boom') } } + const fallback = { foo: 4 } + const { write, read } = makeSlot(undefined) + throws(() => binder.bind('foo', source, write, read, false, fallback), TypeError) +} + +// Both sources in their dead zone still defers to the retry path, then +// resolves from whichever source becomes live. +{ + const binder = new ModuleBinder() + let live = false + const dead = { get foo () { throw new ReferenceError('tdz') } } + const fallback = { get foo () { if (!live) throw new ReferenceError('tdz'); return 5 } } + const { slot, write, read } = makeSlot(undefined) + binder.bind('foo', dead, write, read, false, fallback) + strictEqual(slot.value, undefined, 'deferred while both sources are dead') + live = true + binder.flush() + await Promise.resolve() + strictEqual(slot.value, 5, 'resolved on retry once the fallback became live') +} + +// useFallback (the source.default read) composes with a fallbackSource. +{ + const binder = new ModuleBinder() + const source = {} + const fallback = { 'module.exports': 6 } + const { slot, write, read } = makeSlot(undefined) + binder.bind('module.exports', source, write, read, true, fallback) + strictEqual(slot.value, 6, 'fallbackSource read when both named and default are absent') +}