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

First draft Tech stack file provider #12

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

tinvaan
Copy link
Contributor

@tinvaan tinvaan commented Jan 5, 2024

A first draft implementation of a tech stack file opencodegraph provider. Currently only supports npm packages and .js & .ts source files.

@sqs
Copy link
Member

sqs commented Jan 6, 2024

Awesome! I'll take a look this weekend. Any screenshots to share, or need help getting it actually running end-to-end?

@tinvaan
Copy link
Contributor Author

tinvaan commented Jan 6, 2024

Awesome! I'll take a look this weekend. Any screenshots to share, or need help getting it actually running end-to-end?

Yeah, I'm trying to get the annotations show up on the playground. It seems alright in a unit test.

@tinvaan
Copy link
Contributor Author

tinvaan commented Jan 7, 2024

Awesome! I'll take a look this weekend. Any screenshots to share, or need help getting it actually running end-to-end?

Yeah, I'm trying to get the annotations show up on the playground. It seems alright in a unit test.

The issue seems to be with loading server side modules(viz. fs, path & yaml) on the browser. For this provider, we need a way to read the settings defined in a techstack.yml file.

failed to get annotations: Error: Module "path" has been externalized for browser compatibility.
Cannot access "path.resolve" in client code. 
See https://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

I read up about polyfilling node libraries in vite.config as a workaround but I'm not too familiar with the frontend stack (vite etc), any suggestions on a workaround @sqs ?

@tinvaan
Copy link
Contributor Author

tinvaan commented Jan 7, 2024

Okay, got a POC for the provider up and running on the web playground - I'll polish the PR shortly.

image

Copy link
Member

@sqs sqs left a comment

Choose a reason for hiding this comment

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

Thank you! Some comments and questions. This is super helpful for designing the OpenCodeGraph provider API (which is super early).

(BTW, I have a big refactor of the OpenCodeGraph API in the provider-docs branch. It should be a very small modification to your provider code here, so that need not block merging this.)

client/web-playground/src/demo/settings.ts Outdated Show resolved Hide resolved
const result = techstack.capabilities({}, SETTINGS)

expect(result).toBeDefined()
expect(result).toStrictEqual<CapabilitiesResult>({
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd want the techstack file to show up not in code files but in config and build files, so maybe something like:

  • .* (dotfiles)
  • **/package.json
  • **/*.bazel
  • **/README.md
  • techstack.y?(a)ml of course
  • others like that?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, or is the intent to show it alongside the require or import statements in JS/TS files? Can you add a README describing the expected behavior briefly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was the intention for the first draft implementation(only imports), it would be too complex and noisy to annotate rest of the code.

Annotating build and config files also makes sense, but I'd like to introduce that in a future iteration. I've intentionally kept this first draft minimal. Does this approach sound okay?

provider/techstack/index.ts Outdated Show resolved Hide resolved
* @param filename - techstack yml filename
* @returns parsed yaml object
*/
async function load(filename: string): Promise<TSF> {
Copy link
Member

Choose a reason for hiding this comment

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

Being able to automatically infer the techstack.yml file seems quite important here. Unfortunately, OpenCodeGraph does not yet have a way to help you do this. I was thinking that providers could say in their capabilities response something like "please also send me the contents of the following files with each annotations request: techstack.yml, ...". What do you think about that approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I entirely follow you here but here are my thoughts.

I didn't intend to automatically infer the techstack.yml file - it's set explicitly in the provider settings and the load() method simply references that in L59 and I find the explicitness in this design better.

const report = await load(settings.yaml)

That said, it's maybe not impossible to automatically infer a techstack.yml file for a respository and supplement it with an appropriate override in provider settings. You could also introduce an optional member in the AnnotationsParams interface that sends any techstack.yml data for each annotation request.

However, I don't see any real gain in these approaches vs the current settings driven design. Just my 2 cents.

Copy link
Contributor Author

@tinvaan tinvaan Jan 9, 2024

Choose a reason for hiding this comment

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

Fyi, I've dropped all path resolutions and I'm now adhering to reading techstack.yml content from an absolute path provided in the provider settings. I'll document this in a README, see the below commits.

@@ -6,6 +6,9 @@
// "https://sourcegraph.test:3443/.api/opencodegraph": true,
// "../../../../../../provider/hello-world/dist/index.js": true,
"https://opencodegraph.org/npm/@opencodegraph/provider-hello-world": true,
"https://opencodegraph.org/npm/@opencodegraph/provider-techstack": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sqs can you help me host this provider at opencodegraph.org? how can I test the provider on VSCode locally?

Copy link
Member

Choose a reason for hiding this comment

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

opencodegraph.org/npm/... proxies any npm module. So it should already work!

@tinvaan tinvaan changed the title [WIP] Tech stack file provider First draft Tech stack file provider Jan 9, 2024
@tinvaan tinvaan marked this pull request as ready for review January 9, 2024 12:43
@tinvaan tinvaan requested a review from sqs January 9, 2024 12:43
@sqs
Copy link
Member

sqs commented Jan 22, 2024

Thank you @tinvaan. I will follow up on this when I finish a big migration in the API. You are the first to contribute something, which is awesome, but it means you will definitely feel the rapid change during this experimental period, sorry!

@tinvaan
Copy link
Contributor Author

tinvaan commented Jan 23, 2024

Thank you @tinvaan. I will follow up on this when I finish a big migration in the API. You are the first to contribute something, which is awesome, but it means you will definitely feel the rapid change during this experimental period, sorry!

That's fine @sqs , thanks for the heads up re the migration. Before your changes land, could you tag & release the current main branch? I'm planning to implement another provider and would be easier if I have a locked down release version that I can work with.

@tinvaan tinvaan force-pushed the providers/stackshare branch 2 times, most recently from e11983d to 4b7a1c6 Compare May 27, 2024 06:14
@tinvaan tinvaan force-pushed the providers/stackshare branch from 4b7a1c6 to 4d90f8f Compare May 27, 2024 10:14
@tinvaan tinvaan force-pushed the providers/stackshare branch from 4d90f8f to 590a4ea Compare May 27, 2024 10:54
@sqs
Copy link
Member

sqs commented May 28, 2024

➞  pnpm run test:unit                                                                                                                                                                              
 WARN  Unsupported engine: wanted: {"node":">=20.10.0"} (current: {"node":"v18.13.0","pnpm":"8.14.1"})

> @openctx/root@ test:unit /Users/harish/Workspaces/oss/sourcegraph/opencodegraph
> vitest run

Error: Build failed with 1 error:
renderer/+config.ts:3:20: ERROR: [plugin: vike-esbuild] Could not resolve "/logomark-v0.svg"
 ELIFECYCLE  Command failed with exit code 1.

I think this is from an old version of the web/ code. I recommend rebasing your PR on top of latest main.

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