Fix pnp indexing #262
Fix pnp indexing #262
Conversation
Yarn patches Typescript (certain versions) to support PnP dependency management strategy. The easiest thing we can do is to move our own build to Yarn Berry so that the patch is applied automatically, and we can support both node_modules and PnP approach
|
The tests are broken in a non-trivial way. There are some basic things like version of typescript that got bumped, but more importantly we seem to have lost package information for symbols imported from dependencies. |
| @@ -0,0 +1,3 @@ | |||
| nodeLinker: pnp | |||
There was a problem hiding this comment.
Curious, what makes it harder to use the patch package without migrating the repo to yarn berry with PnP enabled? (maybe it's impossible, but it's clear from the outside why 🙂)
There was a problem hiding this comment.
There are several things going on:
- I switched this projecto pnp to have a reliable integration test (run indexer on its own codebase)
- The compat plugin is only present in the Berry lineage of Yarn it seems - https://github.com/yarnpkg/berry/blob/master/packages/plugin-compat/sources/index.ts#L15C22-L15C31 which likely means it won't work with older Yarn. Just adding the plugin to devDependencies (or dependencies) did nothing
- On the Yarn 1.x the TS is not patched - only ts-loader for Webpack is: https://classic.yarnpkg.com/en/docs/pnp/getting-started#search
All this hairball means that in order to apply the typescript patch I had to switch to Yarn 3.x in this project (so that TS patch is applied automatically), but that led ot other issues needing fixing.
I'm currently stuck at what appears to be that something with regards to module resolution is still broken - but it no longer happens in tsconfig.json (which was the original problem), but it happens when analysing the code itself - imports from modules just become local definitions, which is wrong.
There was a problem hiding this comment.
I'll just use this thread as a brain dump
I have a hunch that current issue with missing imports is related to PnP in the following way:
- We associate packages with filenames when we process node_modules as part of the indexing: https://github.com/sourcegraph/scip-typescript/blob/main/src/Packages.ts#L10
- Because there are no node_modules, the imported files are never processed, and therefore never associated with a package name
- Because it's not a normal package resolution made by TS, the patch doesn't apply to it
As a stop gap solution, we could just change nodeLinker to node-modules before yarn install.
Yarn docs warn against it, and we will be operating on a dirty codebase, but IMO this will be so much quicker to fix.
Closes #259
We're switching this build itself to Yarn Berry, which will automatically apply the TypeScript patch to support PnP: https://github.com/yarnpkg/berry/blob/master/packages/plugin-compat/extra/typescript/gen-typescript-patch.js#L123-L129
This seemed to be the easiest way of doing this.
Test plan