fix(node): full re-export fallback for unresolvable member re-exports#34689
Merged
Conversation
…orts PR #34363 taught the CJS analyzer to handle the `module.exports = require("X").MEMBER` shape by narrowing the wrapper's named exports to those statically attached to MEMBER inside X. That works when the attachments are top-level, but graphql-tag@2 (the canonical motivating package) ships a UMD bundle that wraps its `exports.gql = …` / `gql.* = …` assignments inside the factory IIFE and builds the value through a namespace alias. Static narrowing finds nothing, so `import { gql } from "npm:graphql-tag"` still failed with "does not provide an export named 'gql'". When the member's attached names can't be determined statically, re-export the inner module wholesale instead of advertising nothing. This matches Node, which over-approximates `require(X).Y` to all of X's named exports. The narrow path is unchanged for the cases where the attachments are statically known, so unrelated names still don't leak through there. Closes #16708.
bartlomieju
commented
Jun 2, 2026
bartlomieju
left a comment
bartlomieju
left a comment
Member
Author
There was a problem hiding this comment.
Reviewed the implementation in context. Small, well-targeted change with a genuinely discriminating test. Overall LGTM pending green CI.
Correctness
- The
Ok(None)branch inresolve_member_reexportsnow collects the inner specifier instead of silently dropping it, and both call sites feed those into the existing wholesale re-export path. The narrow path (Ok(Some(props))) is untouched, so statically-resolvable members still don't leak unrelated names. - No infinite-loop risk: each
analyze_reexportscall seedshandled_reexportswith the entry and dedups inner specifiers. defaultis consistently filtered on both the member path and the wholesale path, so it won't leak as a named export.
Test
- Importing all three of
gql,resetCaches,disableFragmentWarnings(not justgql) is what makes this discriminating: if static narrowing had partially resolved to onlygql, the other two would fail, so this actually exercises the new fallback. It also asserts runtime values, not just that imports resolve, covering the cjs-to-esm value synthesis. The fixture faithfully mirrors graphql-tag@2's UMD-inside-IIFE-via-namespace-alias shape.
Minor (non-blocking)
- nit: the "resolve members -> if fallback non-empty, re-export wholesale" block is duplicated across
analyze_all_exportsand theanalyze_reexportsloop. Fine to leave, since the two call sites dispatch differently. - The body says
Closes #16708, but this also fixes #25311 (the original graphql-tag report). Worth adding#25311to the closes list so it auto-closes.
Member
Author
|
Thanks for the review.
|
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.
Importing a named export from graphql-tag still failed with "does not
provide an export named 'gql'", even though #34363 added handling for the
module.exports = require("X").MEMBERentry shape that graphql-tag@2uses. That fix narrows the wrapper's named exports to the names
statically attached to MEMBER inside X, which works when the attachments
are top-level but not for graphql-tag's actual inner file: a UMD bundle
that performs its
exports.gql = ...andgql.* = ...assignments insidethe factory IIFE and builds the value through a namespace alias. Static
narrowing finds nothing there, so the wrapper advertised no names at all.
When the member's attached names cannot be determined statically, fall
back to re-exporting the inner module wholesale rather than advertising
nothing. This matches Node, which over-approximates
require(X).Yto theinner module's full set of named exports. The narrow path is unchanged
for the cases where the attachments are statically known, so unrelated
names still do not leak through there.
Closes #16708
Closes #25311