Skip to content

Conversation

@ematipico
Copy link
Member

@ematipico ematipico commented May 28, 2025

Summary

  • Fixes an issue where the ordering of diagnostics wasn't predictable
  • Reduce the usage of memory by not saving the content of .d.ts files that belong to node_modules inside the Workspace

Test Plan

CI should stay green. Updated the existing snapshots.

@ematipico ematipico requested review from a team May 28, 2025 08:43
@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project labels May 28, 2025
@ematipico ematipico force-pushed the perf/dont-save-dts-contents branch 3 times, most recently from afaf28e to 47b22cb Compare May 28, 2025 10:45
@github-actions github-actions bot added the A-LSP Area: language server protocol label May 28, 2025
@ematipico ematipico force-pushed the perf/dont-save-dts-contents branch 2 times, most recently from 9296f5e to 73e5343 Compare May 28, 2025 11:13
@Conaclos
Copy link
Member

Have you run any tests to see if this would save memory on a large project?

});
})
.await
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe an expect() with a message that could point us in the right direction?

project_key,
path: Some(project_path),
watch: true,
watch: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why wouldn't we want to watch it? This is the only place we set it to true (outside tests).

Copy link
Member Author

Choose a reason for hiding this comment

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

ooops this is a leftover :D

let _ = spawn_blocking(scan_project).await;
})
.await
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

@ematipico
Copy link
Member Author

Have you run any tests to see if this would save memory on a large project?

Yes, I tried a lot, and it does help somewhat; however, I believe the majority of memory consumption is due to the module graph and type inference.

The thing is, at the moment, we compute all .d.ts files inside node_modules, even the ones that belong to indirect dependencies. I talked with @arendjr about it, and we agreed with this approach for the time being, and then optimise it later while our inference system and infrastructure gets better.

The main challenge is re-exports, where dependencies can export their dependencies, and this is a very common pattern.

I will update the FAQ section to make sure users are aware of the current works and what to expect.

@ematipico ematipico requested a review from arendjr May 28, 2025 12:25
@arendjr
Copy link
Contributor

arendjr commented May 28, 2025

I guess that test failure makes sense, you'll have to query the module graph instead to verify its presence.

@ematipico ematipico force-pushed the perf/dont-save-dts-contents branch from bb07713 to 1f96894 Compare May 28, 2025 14:39
@ematipico ematipico merged commit 3b9c4cf into main May 28, 2025
11 checks passed
@ematipico ematipico deleted the perf/dont-save-dts-contents branch May 28, 2025 16:49
ematipico added a commit that referenced this pull request May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-CLI Area: CLI A-LSP Area: language server protocol A-Project Area: project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants