Skip to content
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

Fix pnp indexing #262

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,15 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- run: yarn install
- run: npm run test
- run: yarn install && cd snapshots && yarn install
- run: yarn run test
- name: Integration tests
run: |
set -e
yarn run index . && du -h ./index.scip

git clone https://github.com/aidenybai/million.git && cd million && \
yarn run index . && du -h ./index.scip
lint:
runs-on: ubuntu-latest
steps:
Expand Down
9 changes: 8 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,12 @@ dist
node_modules
.eslintcache
yarn-error.log
snapshots/output/**/*.scip
**/*.scip
tsconfig.tsbuildinfo
index.scip

# see https://next.yarnpkg.com/getting-started/qa#which-files-should-be-gitignored
.pnp.*
**/.yarn/*
!**/.yarn/plugins
!**/.yarn/releases
2 changes: 1 addition & 1 deletion .tool-versions
Original file line number Diff line number Diff line change
@@ -1 +1 @@
nodejs 16.7.0
nodejs 16.18.1
874 changes: 874 additions & 0 deletions .yarn/releases/yarn-3.6.0.cjs

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions .yarnrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


yarn-path ".yarn/releases/yarn-1.22.19.cjs"
3 changes: 3 additions & 0 deletions .yarnrc.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
nodeLinker: pnp
Copy link
Member

Choose a reason for hiding this comment

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

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 🙂)

Copy link
Author

Choose a reason for hiding this comment

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

There are several things going on:

  1. I switched this projecto pnp to have a reliable integration test (run indexer on its own codebase)
  2. 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
  3. 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.

Copy link
Author

Choose a reason for hiding this comment

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

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:

  1. 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
  2. Because there are no node_modules, the imported files are never processed, and therefore never associated with a package name
  3. 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.


yarnPath: .yarn/releases/yarn-3.6.0.cjs
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
"publisher": "sourcegraph",
"bin": "dist/src/main.js",
"main": "./dist/src/main.js",
"packageManager": "[email protected]",
"scripts": {
"prettier": "prettier --write --list-different '**/*.{ts,js?(on),md,yml}'",
"prettier-check": "prettier --check '**/*.{ts,js?(on),md,yml}'",
"tslint": "tslint -p tsconfig.json --format stylish",
"eslint": "eslint --cache '**/*.ts?(x)'",
"build": "node ./node_modules/typescript/bin/tsc -b .",
"build": "tsc -b .",
"index": "node dist/src/main.js index",
"test": "uvu -r ts-node/register --ignore dist",
"update-snapshots": "uvu -r ts-node/register --ignore dist --update-snapshots",
"prepare": "cd snapshots && yarn && cd input/multi-project && yarn && cd ../pnpm-workspaces && pnpm install"
Expand Down
4 changes: 2 additions & 2 deletions snapshots/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
"input/*"
],
"dependencies": {
"react": "18.0.0",
"@sourcegraph/tsconfig": "4.0.1"
"@sourcegraph/tsconfig": "4.0.1",
"react": "18.0.0"
},
"devDependencies": {
"@types/react": "17.0.52"
Expand Down
165 changes: 122 additions & 43 deletions snapshots/yarn.lock
Original file line number Diff line number Diff line change
@@ -1,51 +1,130 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1
# This file is generated by running "yarn install" inside your project.
# Manual changes might be lost - proceed with caution!

__metadata:
version: 6
cacheKey: 8

"@sourcegraph/[email protected]":
version "4.0.1"
resolved "https://registry.yarnpkg.com/@sourcegraph/tsconfig/-/tsconfig-4.0.1.tgz#5965ec41771d2ac5b23b6e0d919cee3e70485840"
integrity sha512-G/xsejsR84G5dj3kHJ7svKBo9E5tWl96rUHKP94Y2UDtA7BzUhAYbieM+b9ZUpIRt66h3+MlYbG5HK4UI2zDzw==
"@example/a@workspace:input/multi-project/packages/a":
version: 0.0.0-use.local
resolution: "@example/a@workspace:input/multi-project/packages/a"
languageName: unknown
linkType: soft

"@types/prop-types@*":
version "15.7.4"
resolved "https://registry.yarnpkg.com/@types/prop-types/-/prop-types-15.7.4.tgz#fcf7205c25dff795ee79af1e30da2c9790808f11"
integrity sha512-rZ5drC/jWjrArrS8BR6SIr4cWpW09RNTYt9AMZo3Jwwif+iacXAqgVjm0B0Bv/S1jhDXKHqRVNCbACkJ89RAnQ==
"@example/b@workspace:input/multi-project/packages/b":
version: 0.0.0-use.local
resolution: "@example/b@workspace:input/multi-project/packages/b"
languageName: unknown
linkType: soft

"@types/[email protected]":
version "17.0.52"
resolved "https://registry.yarnpkg.com/@types/react/-/react-17.0.52.tgz#10d8b907b5c563ac014a541f289ae8eaa9bf2e9b"
integrity sha512-vwk8QqVODi0VaZZpDXQCmEmiOuyjEFPY7Ttaw5vjM112LOq37yz1CDJGrRJwA1fYEq4Iitd5rnjd1yWAc/bT+A==
"@sourcegraph/tsconfig@npm:4.0.1":
version: 4.0.1
resolution: "@sourcegraph/tsconfig@npm:4.0.1"
checksum: cd552b47dcf27b59e944dee20ff29e8f96b77c956bc96c7db44910217fb1c978ff410add86c92ff242274f91484b59b27be4980aea25c77fd922b3c5e31f5d05
languageName: node
linkType: hard

"@types/prop-types@npm:*":
version: 15.7.4
resolution: "@types/prop-types@npm:15.7.4"
checksum: ef6e1899e59b876c273811b1bd845022fc66d5a3d11cb38a25b6c566b30514ae38fe20a40f67622f362a4f4f7f9224e22d8da101cff3d6e97e11d7b4c307cfc1
languageName: node
linkType: hard

"@types/react@npm:17.0.52":
version: 17.0.52
resolution: "@types/react@npm:17.0.52"
dependencies:
"@types/prop-types": "*"
"@types/scheduler": "*"
csstype: ^3.0.2
checksum: a51b98dd87838d161278fdf9dd78e6a4ff8c018f406d6647f77963e144fb52a8beee40c89fd0e7e840eaeaa8bd9fe2f34519410540b1a52d43a6f8b4d2fbce33
languageName: node
linkType: hard

"@types/scheduler@npm:*":
version: 0.16.2
resolution: "@types/scheduler@npm:0.16.2"
checksum: b6b4dcfeae6deba2e06a70941860fb1435730576d3689225a421280b7742318d1548b3d22c1f66ab68e414f346a9542f29240bc955b6332c5b11e561077583bc
languageName: node
linkType: hard

"csstype@npm:^3.0.2":
version: 3.0.11
resolution: "csstype@npm:3.0.11"
checksum: 95e56abfe9ca219ae065acb4e43f61771a03170eed919127f558dfa168240867aba7629c8d98a201a0dd06d9a5ce82686f0570031c928516c61816adbc7c877f
languageName: node
linkType: hard

"invalid-package-json@workspace:input/invalid-package-json":
version: 0.0.0-use.local
resolution: "invalid-package-json@workspace:input/invalid-package-json"
languageName: unknown
linkType: soft

"js-tokens@npm:^3.0.0 || ^4.0.0":
version: 4.0.0
resolution: "js-tokens@npm:4.0.0"
checksum: 8a95213a5a77deb6cbe94d86340e8d9ace2b93bc367790b260101d2f36a2eaf4e4e22d9fa9cf459b38af3a32fb4190e638024cf82ec95ef708680e405ea7cc78
languageName: node
linkType: hard

"loose-envify@npm:^1.1.0":
version: 1.4.0
resolution: "loose-envify@npm:1.4.0"
dependencies:
"@types/prop-types" "*"
"@types/scheduler" "*"
csstype "^3.0.2"

"@types/scheduler@*":
version "0.16.2"
resolved "https://registry.yarnpkg.com/@types/scheduler/-/scheduler-0.16.2.tgz#1a62f89525723dde24ba1b01b092bf5df8ad4d39"
integrity sha512-hppQEBDmlwhFAXKJX2KnWLYu5yMfi91yazPb2l+lbJiwW+wdo1gNeRA+3RgNSO39WYX2euey41KEwnqesU2Jew==

csstype@^3.0.2:
version "3.0.11"
resolved "https://registry.yarnpkg.com/csstype/-/csstype-3.0.11.tgz#d66700c5eacfac1940deb4e3ee5642792d85cd33"
integrity sha512-sa6P2wJ+CAbgyy4KFssIb/JNMLxFvKF1pCYCSXS8ZMuqZnMsrxqI2E5sPyoTpxoPU/gVZMzr2zjOfg8GIZOMsw==

"js-tokens@^3.0.0 || ^4.0.0":
version "4.0.0"
resolved "https://registry.yarnpkg.com/js-tokens/-/js-tokens-4.0.0.tgz#19203fb59991df98e3a287050d4647cdeaf32499"
integrity sha512-RdJUflcE3cUzKiMqQgsCu06FPu9UdIJO0beYbPhHN4k6apgJtifcoCtT9bcxOpYBtpD2kCM6Sbzg4CausW/PKQ==

loose-envify@^1.1.0:
version "1.4.0"
resolved "https://registry.yarnpkg.com/loose-envify/-/loose-envify-1.4.0.tgz#71ee51fa7be4caec1a63839f7e682d8132d30caf"
integrity sha512-lyuxPGr/Wfhrlem2CL/UcnUc1zcqKAImBDzukY7Y5F/yQiNdko6+fRLevlw1HgMySw7f611UIY408EtxRSoK3Q==
js-tokens: ^3.0.0 || ^4.0.0
bin:
loose-envify: cli.js
checksum: 6517e24e0cad87ec9888f500c5b5947032cdfe6ef65e1c1936a0c48a524b81e65542c9c3edc91c97d5bddc806ee2a985dbc79be89215d613b1de5db6d1cfe6f4
languageName: node
linkType: hard

"multi-project@workspace:input/multi-project":
version: 0.0.0-use.local
resolution: "multi-project@workspace:input/multi-project"
languageName: unknown
linkType: soft

"pnpm-workspaces@workspace:input/pnpm-workspaces":
version: 0.0.0-use.local
resolution: "pnpm-workspaces@workspace:input/pnpm-workspaces"
languageName: unknown
linkType: soft

"pure-js@workspace:input/pure-js":
version: 0.0.0-use.local
resolution: "pure-js@workspace:input/pure-js"
languageName: unknown
linkType: soft

"react-example@workspace:input/react":
version: 0.0.0-use.local
resolution: "react-example@workspace:input/react"
languageName: unknown
linkType: soft

"react@npm:18.0.0":
version: 18.0.0
resolution: "react@npm:18.0.0"
dependencies:
js-tokens "^3.0.0 || ^4.0.0"
loose-envify: ^1.1.0
checksum: 293020b96536b3c7113ee57ca5c990a3f25649d1751b1c7a3aabd16dff0691fe9f1eed1206616d0906d05933536052037340a0c8d0941ff870b0eb469a2f975b
languageName: node
linkType: hard

[email protected]:
version "18.0.0"
resolved "https://registry.yarnpkg.com/react/-/react-18.0.0.tgz#b468736d1f4a5891f38585ba8e8fb29f91c3cb96"
integrity sha512-x+VL6wbT4JRVPm7EGxXhZ8w8LTROaxPXOqhlGyVSrv0sB1jkyFGgXxJ8LVoPRLvPR6/CIZGFmfzqUa2NYeMr2A==
"snapshots@workspace:.":
version: 0.0.0-use.local
resolution: "snapshots@workspace:."
dependencies:
loose-envify "^1.1.0"
"@sourcegraph/tsconfig": 4.0.1
"@types/react": 17.0.52
react: 18.0.0
languageName: unknown
linkType: soft

"syntax@workspace:input/syntax":
version: 0.0.0-use.local
resolution: "syntax@workspace:input/syntax"
languageName: unknown
linkType: soft
8 changes: 5 additions & 3 deletions src/main.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ function isUpdateSnapshot(): boolean {
return process.argv.includes('--update-snapshots')
}

const snapshotNodeModules = join(process.cwd(), 'snapshots', 'node_modules')
if (!fs.existsSync(snapshotNodeModules)) {
const snapshotNodeModulesExists = fs.existsSync(join(process.cwd(), 'snapshots', 'node_modules'))
const snapshotPnp = fs.existsSync(join(process.cwd(), 'snapshots', '.pnp.cjs'))

if (!snapshotPnp && !snapshotNodeModulesExists) {
throw new Error(
`no such file: ${snapshotNodeModules} (to fix this problem, run 'yarn install' in the snapshots/ directory)`
`Neither node_modules nor .pnp.cjs were found in the snapshots directory (to fix this problem, run 'yarn install' in the snapshots/ directory)`
)
}
const inputDirectory = join(process.cwd(), 'snapshots', 'input')
Expand Down
12 changes: 6 additions & 6 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,12 @@ function defaultCompilerOptions(configFileName?: string): ts.CompilerOptions {
// Not a typo, jsconfig.json is a thing https://sourcegraph.com/search?q=context:global+file:jsconfig.json&patternType=literal
configFileName && path.basename(configFileName) === 'jsconfig.json'
? {
allowJs: true,
maxNodeModuleJsDepth: 2,
allowSyntheticDefaultImports: true,
skipLibCheck: true,
noEmit: true,
}
allowJs: true,
maxNodeModuleJsDepth: 2,
allowSyntheticDefaultImports: true,
skipLibCheck: true,
noEmit: true,
}
: {}
return options
}
Expand Down
Loading