Refactor: Bring over additional types from Onivim 2 #22
Conversation
There was a problem hiding this comment.
Most of these are actually designed to be independent of extension contributions, although they are of course heavily modelled after the requirements of vscode extensions. There seems to be very little of this that's actually used, however. From what I can see there's just Menu, IconTheme and WhenExpr, and all of those could easily be replaced by "dumber" data structures. The rest seems to have been brought along just because it's part of the same "ecosystem" of interconnected parts. On the other hand I did have a long-term goal of replacing more of the contribution data structures with these.
I can see a few different courses of action here:
-
Keep this PR as-is. Although I'd still suggest renaming the
Typesmodule to something more meaningful, as it's almost as generic as a project calledSourceCode.DataStructureswould be slightly more accurate, but still doesn't describe any actual structure. There's also a lot more to these than just what I'd call a "data structure" and certainly a lot more than just "types". -
Move them to a separate package, like
oni-platformperhaps. -
Replace the few data structures that are needed with "dumber" equivalents.
WhenExpr.tcould just be kept as astringfor example, to be parsed later. -
Move all of this into Oni2. I'm not sure why this needs to be a separate package, so to me ti seems like a viable option at least.
-
Separate the "dumb" data structures from the surrounding logic, put them either here or in a separate package, then alias them in Oni2 where the rest of the logic lives.
My preference I think would be to either move ti all into Oni2, as I don't see why it needs to be separate and that would make it much easier to iterate on these things, or to make a separate oni-platform package.
| @@ -0,0 +1,24 @@ | |||
| module Key: { | |||
There was a problem hiding this comment.
This, and the interface, seems to be duplicated in Types.
| (library | ||
| (name Core) | ||
| (public_name vscode-exthost.core) | ||
| (library_flags (-linkall)) | ||
| (libraries luv yojson decoders-yojson) | ||
| (preprocess (pps ppx_deriving.show ppx_deriving_yojson))) |
There was a problem hiding this comment.
Seems like this should have been removed entirely?
|
@glennsl - thanks for the feedback! The reason I split it out is to make it easier to test extensions along with projects in isolation - there are less moving parts when the extension host is split out, so I can validate the behavior of extensions independently of Onivim 2. Ultimately, I'd like to have an integration test suite here - where we validate the top X plugins we care about, along with scripts to provision projects to go with the plugins (ie, for Python, C#, Java, etc etc). Knowing that the Upgrading the extension host in Onivim 2, or validating new extensions, has more cognitive overhead, just because there are more moving parts (ie, if it fails, which layer is to blame?) - removing the extra layers and being able to test in isolation makes it easier, at least for me, to work with. In addition, we have cases where we are tightly coupled to the extension host for language features - I believe this split will help us maintain a clearer architectural boundary. So I do plan on keeping this split - but I like your idea of an |
|
I don't think any of this requires a separate repository though. Like a monorepo, you can have strong separation within a single repository, although you'd still have to reorganize this code just like you're doing now. But it would be much easier to work on changes across repositories, because you don't have to create multiple PRs, wait for CI to merge, sync up dependencies, do releases to avoid resolutions etc. Of course there are other benefits to having s separate repository, like ease of external contribution, issues, licensing and such, but I can't see any of that really applying here.
There might be room to make this clearer by improving the naming, but the root of the problem is that we're trying to come up with general names for interfaces to implementations that are really very specific. |
That's a good point! I'll take a look at bringing this over to the Onivim repo. The downside is we'll have two 'ext host' concepts, until the v2 (the one in this repo) has functional parity with the v1 (the one in Onivim) - and at that point we can completely remove the old 'v1'. But seems reasonable as a transition step. |
Well, it's not the only thing we have two of, so it'll have company at least! 😄 |
In starting to investigate wiring up this library to Onivim 2 (ie, replacing the
ManifestandScannerin Onivim 2 with the one ported over here) - I found there were several other types that needed to be brought over.In particular, in the
Manifest, there is a contributions section which contains information about commands, menu items, and conditions. It seems like the data model for all of those needs to be brought over - so this ports severalCore.Typesfrom Onivim 2:Config,Menu,IconTheme,WhenExpr, as well as some dependencies (KeyedStringMap/KeyedStringTree).There might be other possibilities; like creating a functor or custom parsing strategies for Contributions - but I think these data models are somewhat coupled to the extension contributions, so I'm not sure if it is worth generalizing it to that extent. @glennsl , you might have ideas, though.