Skip to content

Typings Intaller initial work #844

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 22 commits into from
May 30, 2025
Merged

Typings Intaller initial work #844

merged 22 commits into from
May 30, 2025

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented May 2, 2025

This ports the typescript installer code from Strada

Instead of different process we use go routine to queue discovery of the packages install and then another go routine to do actual install.

Here are things that are still not ported and are todo for later

  • Js file names to search typings for - need to exclude source files from external module search - currently this data is not threaded so left it for later
  • custom npm path, safelist path etc that earlier we use to take - review and add the options that we really need
  • Strada use to use events to notify client about typings installation. we need to use notification but will do that later
  • TypingsInstaller currently uses "latest" as version to install typings for, to change this to correct version when we have typings for those published

While working on this, found out current bug in strada where typings already installed are not cached correctly if lockfile version is newer. This i havent fixed but will do that separately later. This means even if say @types/chai is already in our cache location installed, we will reinstall it. (Because package-lock.json structure change)

@sheetalkamat sheetalkamat force-pushed the typingsInstaller branch 2 times, most recently from 228ebe4 to 859598b Compare May 5, 2025 17:31
@sheetalkamat sheetalkamat force-pushed the typingsInstaller branch 12 times, most recently from 41dc074 to 29168d5 Compare May 15, 2025 22:09
@sheetalkamat sheetalkamat marked this pull request as ready for review May 15, 2025 22:58
@Copilot Copilot AI review requested due to automatic review settings May 15, 2025 22:58
Copy link
Contributor

@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 ports the TypeScript installer logic from Strada into a more concurrent design by replacing separate processes with goroutines and extending type acquisition support.

  • Introduces changes to how typings are discovered, cached, and installed.
  • Updates module resolution and project setup to incorporate typings location and type acquisition.
  • Adds new helper methods and fields in several modules (e.g. tsoptions, project, parsedcommandline, resolver) to support the new installer design.

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/tsoptions/tsconfigparsing.go Replaces legacy functions with vfs methods for generating regexes.
internal/testutil/projecttestutil/projecttestutil.go Updates test installer setup and adds NpmInstall behavior using panics for unexpected cases.
internal/project/watch.go Adjusts watched file management by introducing watchType and switching to host.Client() calls.
internal/project/* Extends type acquisition and typings-handling across project construction, resolution, and update.
internal/lsp/server.go Adds typingsLocation to server options and logs its usage.
internal/core/nodemodules.go, internal/module/resolver.go Propagates typings-related fields down to the resolver and module resolution logic.
cmd/tsgo/lsp.go Implements cross-platform logic to derive a global typings cache location.
Comments suppressed due to low confidence (2)

internal/project/discovertypings.go:294

  • Ensure thorough unit tests cover edge cases for removeMinAndVersionNumbers to validate that its behavior matches the legacy regex approach.
func removeMinAndVersionNumbers(fileName string) string {

internal/api/api.go:75

  • [nitpick] If the nil return is intentional as a stub, add an inline comment explaining that the TypingsInstaller is not yet implemented.
func (api *API) TypingsInstaller() *project.TypingsInstaller { return nil }

@sheetalkamat
Copy link
Member Author

looking into test failures

@sheetalkamat sheetalkamat requested a review from jakebailey May 29, 2025 22:14
@sheetalkamat sheetalkamat enabled auto-merge May 30, 2025 20:22
@sheetalkamat sheetalkamat added this pull request to the merge queue May 30, 2025
@jakebailey
Copy link
Member

Works on my machine 😄

Merged via the queue into main with commit acba540 May 30, 2025
23 checks passed
@sheetalkamat sheetalkamat deleted the typingsInstaller branch May 30, 2025 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants