Skip to content

Conversation

@GGomez99
Copy link

Motivation

This PR adds Plug'n'Play support natively to Typescript Go, following this issue: #460
It has been reviewed and supported by @arcanis, the lead maintainer of Yarn, and the original author of Yarn PnP.

Datadog has a frontend monorepo using yarn with over 6k packages, and seeing how TS Strada struggles with our current scaling, we decided to invest time in adding a native Yarn PnP support for Typescript Go.
This PnP implementation has been actively used in the IDE of more than 230 engineers at Datadog, and we're committed to fixing all issues reported to us.

Challenges

We did not integrate it in our CI yet as we still have several packages failing on build mode (most errors seem to be reported in the issues section of TS Corsa). Because the TS Corsa API is not available yet, we also couldn't integrate it properly with a fast lage setup unlike with the TS Strada API.

Changes

It's based on the main changes from the original yarn patch (microsoft/TypeScript@99f3e13) that the community has been maintaining for years throughout Typescript Strada updates, except that we implemented the official PnP specification so it doesn't depend on third-party code.

Implemented features:

  • PnP VFS that handles virtual folders and zip files seamlessly, with caching and fallback to the original vfs if pnp is not available
  • Add PnP API and manifest handling, following the yarn PnP specification
  • Initialize the PnP API as a singleton and VFS for both build and LSP modes
  • Add PnP support when resolving modules in internal/module/resolver.go
  • Add PnP support for auto-imports and completion at internal/modulespecifiers/specifiers.go
  • Add PnP support for root types at internal/core/compileroptions.go
  • Handle zip paths when going to implementation with the LSP
  • Update the baseline testing framework to handle PnP when needed

Missing features:

  • Bubbling up unresolved modules errors from pnpApi.ResolveToUnqualified
  • PnP manifest auto-refresh by watching .pnp.cjs changes

Tests

  • Basic PnP setup
  • Types from transitive dependencies
  • Root types loading from PnP dependencies
  • Completion and autoimports (test framework needs tinkering for internal/fourslash)

@GGomez99
Copy link
Author

@microsoft-github-policy-service agree company="Datadog"

@GGomez99
Copy link
Author

I see my tests are not passing in CI, looking into it 🙇

Comment on lines +12 to +30
var (
isPnpApiInitialized atomic.Uint32
cachedPnpApi *PnpApi
pnpMu sync.Mutex
// testPnpCache stores per-goroutine PnP APIs for test isolation
// Key is goroutine ID (as int)
testPnpCache sync.Map // map[int]*PnpApi
)

// getGoroutineID returns the current goroutine ID
// It is usually not recommended to work with goroutine IDs, but it is the most non-intrusive way to setup a parallel testing environment for PnP API
func getGoroutineID() int {
var buf [64]byte
n := runtime.Stack(buf[:], false)
idField := strings.Fields(strings.TrimPrefix(string(buf[:n]), "goroutine "))[0]
id, _ := strconv.Atoi(idField)
return id
}

Copy link
Member

Choose a reason for hiding this comment

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

All of this global / goroutine local storage is something that won't work. If we are storing something, it needs to be attached to a Program, Project, a Host, etc, not global. Parsing out the goroutine ID is definitely a bad idea, especially given our LSP can handle multiple requests at the same time from multiple goroutines and so on.


pnpApi := &PnpApi{fs: fs, url: filePath}

manifestData, err := pnpApi.findClosestPnpManifest()
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this init. Surely one needs to be able to load multiple projects with differing pnp info at the same time? All of this info really does need to be handled differently.

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.

2 participants