Handle lock file 3 version when caching the typings ensuring we can reuse already installed packages #61730
Merged
sheetalkamat merged 3 commits intomainfrom May 21, 2025
Merged
Handle lock file 3 version when caching the typings ensuring we can reuse already installed packages #61730sheetalkamat merged 3 commits intomainfrom
sheetalkamat merged 3 commits intomainfrom
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the typings installer caching mechanism to support lock file version 3 by handling both the legacy "dependencies" and the new "packages" properties from package-lock files.
- Updated the NpmLock interface and modified dependency lookup logic in the installer core.
- Added new unit tests to validate both expired and non-expired cache entry scenarios with lockFile3.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/baselines/reference/tsserver/typingsInstaller/non-expired-cache-entry-lockFile3.js | New baseline test output for non-expired cache entry handling using lock file 3 |
| src/typingsInstallerCore/typingsInstaller.ts | Adjusted NpmLock interface and conditional logic to support both legacy and lockFile3 formats |
| src/testRunner/unittests/tsserver/typingsInstaller.ts | Added tests for expired and non-expired cache entries when using lock file 3 |
Comments suppressed due to low confidence (2)
src/typingsInstallerCore/typingsInstaller.ts:341
- [nitpick] Consider refactoring this lookup into a more explicit conditional structure to avoid the non-null assertion on 'npmLock.dependencies' and improve overall readability.
const info = npmLock.packages && getProperty(npmLock.packages, `node_modules/${key}`) || getProperty(npmLock.dependencies!, key);
src/testRunner/unittests/tsserver/typingsInstaller.ts:1415
- [nitpick] Consider adding explicit assertions after running pending installs to verify that typings are installed or skipped as expected in both the 'expired' and 'non-expired' cache entry scenarios.
const host = TestServerHost.createServerHost(
andrewbranch
approved these changes
May 21, 2025
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
While porting this portion of code to Corsa, i found that we are not handling lock file 3 format which means the installed typings are not honored and considered to be out of version scope when typings installer initializes. This results in reinstalling the packages everytime that package is required.