feat: add new lsp extension to send dependency information from client to servers #338
Conversation
types/notifyDependencyPaths.ts
Outdated
|
|
||
| export interface NotifyDependencyPathsParams { | ||
| moduleName: string | ||
| programmingLangugage: string |
There was a problem hiding this comment.
typo. Also, "prog lang" may be misleading. This is more like a "mode" or "kind" which triggers heuristics that look at the workspace. In other places (Lambda, SAM CLI, Toolkits, even CodeWhisperer), it's called a "runtime".
For example, "node" is not a programming language, but a node project involves js + ts. Which "programming language" would we specify there?
| programmingLangugage: string | |
| runtime: string |
There was a problem hiding this comment.
That's a good point, what about runtimeLanguage I still think we need the language information and not the runtime
For a node project with both js and ts files, the client can send one notification for each language, imo this is not redundant but rather being explicit in the communication. I am open to discuss
types/notifyDependencyPaths.ts
Outdated
| moduleName: string | ||
| programmingLangugage: string | ||
| files: string[] | ||
| dirs: string[] |
There was a problem hiding this comment.
Do we need a depth parameter?
| dirs: string[] | |
| /** Absolute paths to directories to watch(?) */ | |
| dirs: string[] |
There was a problem hiding this comment.
Good point, at least right now I would rather keep that detail out of scope and let the servers decide on depth. It would be easy to add an extra field in the future
types/notifyDependencyPaths.ts
Outdated
| @@ -0,0 +1,10 @@ | |||
| export const NOTIFY_DEPENDENCY_PATHS_NOTIFICATION_METHOD = 'aws/notifyDependencyPaths' | |||
There was a problem hiding this comment.
(off topic: uppercase is not gaining anything here except a longer, more awkward name.)
There was a problem hiding this comment.
I tried following the existing convention to keep a unified approach but indeed an unwanted side effect is the lengthy constant name. Though there is a clear benefit of having this string in a constant since it will be used by runtimes as well as by clients
types/notifyDependencyPaths.ts
Outdated
| @@ -0,0 +1,10 @@ | |||
| export const NOTIFY_DEPENDENCY_PATHS_NOTIFICATION_METHOD = 'aws/notifyDependencyPaths' | |||
|
|
|||
| export interface NotifyDependencyPathsParams { | |||
There was a problem hiding this comment.
It's difficult to understand what this protocol is needed for and how to use it. Some context, proper documentation may help.
There was a problem hiding this comment.
+1
I expect some discussions on this PR so I didn't add any documentation just yet since things are fluid at the moment
There was a problem hiding this comment.
Also, notify dependency paths about what? Is this a change notification that the dependency paths have been updated? If so we can mimic https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeWorkspaceFolders and call it
didChangeDependencyPaths instead?
types/notifyDependencyPaths.ts
Outdated
| files: string[] | ||
| /** Absolute paths to dependency directories*/ | ||
| dirs: string[] |
There was a problem hiding this comment.
What is the difference between { files: string[], dirs: string[]} and {paths: string[]}?
|
Thanks for the updates, looks good to me! |
| /** Name of the module being processed */ | ||
| moduleName: string | ||
| /** Programming language runtime (e.g., 'javascript', 'python', 'java') */ | ||
| runtimeLanguage: string |
There was a problem hiding this comment.
For a node project with both js and ts files, the client can send one notification for each language, imo this is not redundant but rather being explicit in the communication. I am open to discuss
I thought we discussed that the state for dependency tracking was defined exactly once for a workspace? Thus multiple requests overwrite the previous request.
If that's not the case, then the docstring here should mention "the client must send one notification for each language".
Problem
Servers don't have in depth information regarding dependencies on the client side
Solution
Define new interface that allows clients to send dependency information to servers
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.