Skip to content

Handle lock file 3 version when caching the typings ensuring we can reuse already installed packages #61730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 21, 2025

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented May 19, 2025

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.

@Copilot Copilot AI review requested due to automatic review settings May 19, 2025 16:55
@github-project-automation github-project-automation bot moved this to Not started in PR Backlog May 19, 2025
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 19, 2025
Copy link

@Copilot 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 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(

@github-project-automation github-project-automation bot moved this from Not started to Needs merge in PR Backlog May 21, 2025
@sheetalkamat sheetalkamat merged commit b504a1e into main May 21, 2025
57 of 58 checks passed
@sheetalkamat sheetalkamat deleted the tiLockFile branch May 21, 2025 20:58
@github-project-automation github-project-automation bot moved this from Needs merge to Done in PR Backlog May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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