-
Notifications
You must be signed in to change notification settings - Fork 13
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
Refactor project structure #76
Conversation
Makes sense.
Great callout.
It doesn't need to be detailed, but a high-level outline (which also links to sub-readmes) in the top-level readme should be part of this PR. It's hard for contributors to fish around in sub-directories to piece together a mental model of the architecture. |
I ran into issues with this in the past around the dependencies between the different Even better, I would prefer if we don't re-export |
ProtocolRequestType, | ||
} from 'vscode-languageserver' | ||
InlineCompletionRegistrationOptions, | ||
} from './lsp' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there still a copy/paste here from the 3.18 branch of vscode-languageserver
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, 3.18 LSP is not yet published
Do you remember what was the issue exactly and how to reproduce it? Currently I don't re-export We likely don't have to depend on specific packages like
Apparently, LSP metaModel is actually generated from typescript code, microsoft/language-server-protocol#1707 (comment). I think, generating typescript from it again is unnecessary step, as we can use types as a source of truth directly. Also looking at history of metaModel file, metamodel was out of date/sync with Typescript definition. |
protocol/package.json
Outdated
{ | ||
"name": "@amzn/language-server-runtimes-impl", | ||
"private": true, | ||
"description": "Helper file to flatten import paths in dependant packages", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative is to publish to npm from out
directory, but I'm not convinced which way is better https://davidwells.io/blog/publishing-flat-npm-packages-for-easier-import-paths-smaller-consumer-bundle-sizes
So I preferred explicit config here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in favour of publishing from /out
folder and using helper scripts instead of aux files.
Problem
Currently repository structure does not correctly reflect the architecture of the Runtimes project. This is confusing when we need to reason about the implementation and does not represent intention of runtime implementations and communication protocols. Implementation of specific features is also done mixing core concepts.
Solution
Refactor project structure and separate core components of Runtimes project into logical submodules.
I propose to re-organise code into 4 modules:
protocol
- JSON-RPC-based Runtime protocol implementation in Typescript, which defines the communication between Runtime and Runtime Clients (e.g. AWS Toolkit extension). Contains only Typescript types, without implementations.server-interface
- defines interfaces of features, that Runtime provides to Runtime Servers implementors. Depends onprotocol
. Contains only types definitions.Server
and features interfaces live here.runtimes
- contains implementation of several runtimes (standalone, webworker) and features, that are exposed to Runtime Servers developed by Server implementors. Relies on bothprotocol
andserver-interface
modules.testing
- exposes testing helper for server developers (not modified in this PR).Proposed separation allows us to design upcoming features using mental model and representation of project architecture explicitly.
Explicit dependency of VSCode LSP is implemented in code
Dependency of VSCode LSP is implemented in code explicitly. 2 new dependencies are installed explicitly
vscode-languageserver-protocol
andvscode-languageserver-types
. Inprotocol
module, whole LSP protocol and types are re-exported. The rest of codebase now depends onLSP
part ofprotocol
.Since Runtimes protocol uses LSP as a baseline and extends it where required, Runtime Protocol can be seen as an augmented LSP. We design our extensions to be compatible with standard LSP.
One of the features that we expose to server implementors as part of Server interface is LSP message passing feature. Implementation-wise, however, we do not expose the raw LSP connection used by Runtime to communicate with Client, Instead LSP feature in Runtime proxying LSP messages to Servers, mimicking standard LSP messages as much as possible, and we pass LSP messages payload mostly unmodified. This implies that LSP feature also exposes LSP protocol to Server implementors. That's why in
server-interface
module I propose to re-export whole ofLSP
protocol as well.This change makes LSP protocol an integral part of Runtimes protocol, and allows us to directly control specific version of LSP on which Runtime
protocol
andserver-interface
depend.Added benefit of this, is that Servers implementors in https://github.com/aws/language-servers package can now stop relying on types and protocols defined in
vscode-languageserver
package and use interfaces defined in our protocol as direct dependency and a source of truth. Example of applying this change can be seen here: aws/language-servers@main...consume_runtimes_protocol.vscode-languageserver
will be transient dependency for Server implementations, but it will be transient dependency now.The risk of this approach is that we re-exporting whole surface of 3rd party package now, making us responsible for maintaining this relationship. Also re-exporting makes dependency tree less obvious for downstream dependency packages, developed in https://github.com/aws/language-servers.
Key changes
protocol
andserver-interface
.runtimes
directory.protocol
directory.server-interface
directory.import { CredentialsProvider } from '@aws/language-server-runtimes/out/features'
will change toimport { CredentialsProvider } from '@aws/language-server-runtimesfeatures'
Potential improvements/follow-ups
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.