Skip to content

Conversation

@asgerf
Copy link
Contributor

@asgerf asgerf 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.

@asgerf asgerf added the no-change-note-required This PR does not need a change note label Nov 21, 2025
@github-actions github-actions bot added the JS label Nov 21, 2025
@asgerf asgerf marked this pull request as ready for review November 25, 2025 13:24
@asgerf asgerf requested a review from a team as a code owner November 25, 2025 13:24
Copilot AI review requested due to automatic review settings November 25, 2025 13:24
Copilot finished reviewing on behalf of asgerf November 25, 2025 13:26
Copy link
Contributor

Copilot AI 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

Copilot AI Nov 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

@tausbn tausbn 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.)

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

Labels

JS no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants