Skip to content

Conversation

@sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented May 20, 2025

Time to updategraph went from 276173.6424ms to 33199.64829999999ms

…ss we have project references in node_modules
Copilot AI review requested due to automatic review settings May 20, 2025 20:48
@github-project-automation github-project-automation bot moved this to Not started in PR Backlog May 20, 2025
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 20, 2025
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 adjusts how .d.ts files in node_modules are handled by only resolving their source when at least one project reference from node_modules has been encountered.

  • Introduces a hasResolvedReferencencesInNodeModules flag to gate lookups of .d.ts sources in node_modules
  • Adds a guard in getSourceOfProjectReferenceRedirect to skip node_modules paths until the flag is set
  • Sets the flag to true once a project reference in node_modules is actually redirected
Comments suppressed due to low confidence (1)

src/compiler/program.ts:3827

  • Consider adding unit tests to verify that .d.ts source resolution is skipped for node_modules paths until a project reference in node_modules is encountered, and then allowed afterwards.
if (!hasResolvedReferencencesInNodeModules && pathContainsNodeModules(path)) return undefined;

let projectReferenceRedirects: Map<Path, ResolvedProjectReference | false> | undefined;
let mapFromFileToProjectReferenceRedirects: Map<Path, Path> | undefined;
let mapFromToProjectReferenceRedirectSource: Map<Path, SourceOfProjectReferenceRedirect> | undefined;
let hasResolvedReferencencesInNodeModules = false;
Copy link

Copilot AI May 20, 2025

Choose a reason for hiding this comment

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

There's a typo in the variable name hasResolvedReferencencesInNodeModules. Consider renaming it to hasResolvedReferencesInNodeModules for clarity and consistency.

Suggested change
let hasResolvedReferencencesInNodeModules = false;
let hasResolvedReferencesInNodeModules = false;

Copilot uses AI. Check for mistakes.
let projectReferenceRedirects: Map<Path, ResolvedProjectReference | false> | undefined;
let mapFromFileToProjectReferenceRedirects: Map<Path, Path> | undefined;
let mapFromToProjectReferenceRedirectSource: Map<Path, SourceOfProjectReferenceRedirect> | undefined;
let hasResolvedReferencencesInNodeModules = false;
Copy link

Copilot AI May 20, 2025

Choose a reason for hiding this comment

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

[nitpick] This flag controls when to start resolving .d.ts sources from node_modules. Adding a brief comment above its declaration would improve maintainability and help future readers understand its purpose.

Suggested change
let hasResolvedReferencencesInNodeModules = false;
// Tracks whether `.d.ts` sources from `node_modules` have been resolved.
let hasResolvedReferencencesInNodeModules = false;

Copilot uses AI. Check for mistakes.
Comment on lines 3825 to 3828
function getSourceOfProjectReferenceRedirect(path: Path) {
if (!isDeclarationFileName(path)) return undefined;
if (!hasResolvedReferencencesInNodeModules && pathContainsNodeModules(path)) return undefined;
if (mapFromToProjectReferenceRedirectSource === undefined) {
Copy link

Copilot AI May 20, 2025

Choose a reason for hiding this comment

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

[nitpick] The pathContainsNodeModules check appears in multiple places. Extracting it into a well-named helper (e.g., isInNodeModules(path)) could reduce duplication and improve readability.

Suggested change
function getSourceOfProjectReferenceRedirect(path: Path) {
if (!isDeclarationFileName(path)) return undefined;
if (!hasResolvedReferencencesInNodeModules && pathContainsNodeModules(path)) return undefined;
if (mapFromToProjectReferenceRedirectSource === undefined) {
function isInNodeModules(path: Path): boolean {
return pathContainsNodeModules(path);
}
function getSourceOfProjectReferenceRedirect(path: Path) {
if (!isDeclarationFileName(path)) return undefined;
if (!hasResolvedReferencencesInNodeModules && isInNodeModules(path)) return undefined;
if (mapFromToProjectReferenceRedirectSource === undefined) {

Copilot uses AI. Check for mistakes.
@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 20, 2025

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@sheetalkamat sheetalkamat marked this pull request as draft May 20, 2025 20:55
@typescript-bot
Copy link
Collaborator

typescript-bot commented May 20, 2025

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/165141/artifacts?artifactName=tgz&fileId=B7ACC15D2E3B3FDEB41D55A6E5EAD826AFF18FBDD70B466821AD6189B9893DA502&fileName=/typescript-5.9.0-insiders.20250520.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.9.0-pr-61739-2".;

@github-project-automation github-project-automation bot moved this from Not started to Done in PR Backlog May 21, 2025
@sheetalkamat sheetalkamat deleted the referencesOptimize branch May 21, 2025 21:43
@microsoft microsoft locked as resolved and limited conversation to collaborators Nov 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants