Skip to content

Conversation

@asgerf
Copy link
Contributor

@asgerfasgerf commented Nov 21, 2025

ExportDeclaration.getSourceNode and .exportsAs were global because they look through re-exports.

This PR splits these into a "direct" version that is local and ignores re-exports, and a non-direct version that is global and looks through re-exports.

One of the use-cases for the direct version is for API graphs to make a local over-approximation of the set of def-nodes we need, so that API::Node can become local.

@asgerfasgerf added the no-change-note-required This PR does not need a change note label Nov 21, 2025
@asgerfasgerf marked this pull request as ready for review November 25, 2025 13:24
@asgerfasgerf requested a review from a team as a code ownerNovember 25, 2025 13:24
CopilotAI review requested due to automatic review settings November 25, 2025 13:24
Copy link
Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the module export system to distinguish between direct exports and re-exports, supporting better locality analysis for API graphs. The changes split exportsAs and getSourceNode into "direct" variants (local, ignoring re-exports) and non-direct variants (global, looking through re-exports).

  • Added exportsDirectlyAs and getDirectSourceNode predicates for local export analysis
  • Introduced reExportsAs and getReExportedSourceNode in ReExportDeclaration for tracking re-exported values
  • Refactored type-only export handling and simplified OriginalExportDeclaration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

/**
* Holds if this re-export destination ultimately re-exports `v` (from another module)
Copy link

CopilotAINov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "this re-export destination" but it should say "this re-export declaration" instead. The term "destination" is confusing here - the declaration is the source of the re-export, not the destination.

Copilot generated this review using guidance from repository custom instructions.
Copy link
Contributor

@tausbntausbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! The uppercase E in ReExport kind of makes my eye twitch, but I'm not going to complain loudly about it. (I would prefer Reexport -- or Reëxport if we want to go full New Yorker.)

@asgerfasgerf merged commit d8027fb into github:mainNov 27, 2025
19 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JSno-change-note-requiredThis PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@asgerf@tausbn