fix: resolve 'ambiguous' chained export * via explicit fallback - #282
Draft
FauxFaux wants to merge 2 commits into
Draft
fix: resolve 'ambiguous' chained export * via explicit fallback#282FauxFaux wants to merge 2 commits into
export * via explicit fallback#282FauxFaux wants to merge 2 commits into
Conversation
…lback A name that reaches an aggregator through more than one `export *` chain bottoming out at the same module is dropped from the aggregate namespace as ambiguous, so nodejs#171 pointed its setter at the defining module's own namespace instead. Re-express that replacement read as a fallback: the setter reads the aggregate namespace first and consults the defining module's namespace only when the aggregate yields `undefined` or is in its temporal dead zone. For collided names this is equivalent — the aggregate read always yields `undefined` for them, so control always reaches the fallback — but it collapses the two setter shapes (read the aggregate / read the defining module) into one uniform strategy, read-primary-with-optional-fallback, and states the intent more faithfully: the aggregate is authoritative, the defining module is where to go when the aggregate cannot answer. `ModuleBinder.bind` gains the optional fallback source, pinned by unit tests; a non-ReferenceError from the primary still propagates, and a fallback that is itself unavailable still defers to the existing retry path.
A name re-exported through `export *` from a module caught in a circular import could come back `undefined` under the asynchronous loader. Post-order evaluation runs an aggregating barrel's wrapper before the defining leaf's wrapper body, so the binding is still in its temporal dead zone on the aggregate namespace when the wrapper snapshots it. `ModuleBinder` deferred the read and only recovered it on a later async retry, after a synchronous importer had already read `undefined`. The defining module itself has already hoisted its function/var bindings by then, and the previous commit's fallback read is exactly the tool to reach them: widen it from same-origin collision names to every `export *`-sourced name, built at first sight of the name instead of rebuilt on collision. The collision rebuild becomes redundant — the setter installed up front already falls back to the right defining module — and genuinely ambiguous names stay excluded per ResolveExport. The test reproduces the typebox shape from the issue without the dependency: a 4-module fixture (top -> enter -> leaf <-> barrel) whose cycle makes the barrel's wrapper evaluate while the leaf's bindings are in their temporal dead zone. It passes without the loader and fails under it without this change.
FauxFaux
marked this pull request as draft
August 18, 2026 13:30
Author
|
A further review has found issues with this approach, notably that the fallback module is not hooked. Which doesn't matter at all for my usecase (typescript with opentelemetry, where there's no instrumenter inside typescript), but is a real bug we probably shouldn't plaster over. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #269. Partially reworks #171 / #263.
Full disclosure: This change was significantly AI assisted, hence the extensive comment churn and unit tests. I do believe it to be reasonable, but am far from a domain expert. It is additive, and does not change any existing tests, so I believe it is worth raising. This body is fully human.
When there are two levels of
export *, and a cyclic import, the previous code would handle this as if there was a truly ambiguous doubleexport *, and remove the export (as specified). This differs from the nodejs (and presumably specification) behaviour, where it is allowable to fallback to the original defining(?) namespace.This is broken into two commits: one attempts to re-work the fix in #171 by making this fallback behaviour explicit, then the second extends the fallback to apply (instead of erasure) in the typebox case.
This second commit, while ugly in the diff, is mostly just moving code around to handle an extra
ifcondition, not adding or removing any weird code. It is now valid to do this because of the new fallback behaviour. Focus on thisaddSetterchange.