-
Notifications
You must be signed in to change notification settings - Fork 274
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
Extension Host: Send initialization data #189
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
5975e45
Initial integration
bryphe fcd22fd
Revert vsync change
bryphe ecddc73
Start adding extension host client Reason API
bryphe 1ed187f
Initial ExtensionHost client start-up
bryphe f897ecf
Get extension host running
bryphe ceb832c
Hook up entry point to process messages
bryphe 1213055
Add basic test case to verify we get 'Ready' message from extension host
bryphe 359ab2a
Formatting
bryphe eaf0f58
Tweak runner
bryphe 77cd75e
Factor protocol out
bryphe 78cace3
Fix ExtHostProtocol path
bryphe 038a725
Formatting
bryphe d8d0f2a
Merge master
bryphe 7b96e01
Fix merge conflict
bryphe 5370223
Start stubbing out InitDat
bryphe 613eccf
Get ExtensionHostInitData compiling
bryphe da06404
Formatting
bryphe 9d3cd93
Debugging exception on startup
bryphe 70968a8
Merge branch 'master' into exthost/initialization-data
bryphe 99a78bd
Refactor interface, test for 'initialized' event
bryphe f7f58ad
Formatting
bryphe 9ea3880
Remove logging
bryphe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/* | ||
* ExtensionHostInitData.re | ||
* | ||
* Module documenting the initialization data required for spinning up | ||
* and activating the extension host | ||
*/ | ||
|
||
open Oni_Core; | ||
|
||
module ExtensionInfo = { | ||
[@deriving (show({with_path: false}), yojson({strict: false, exn: true}))] | ||
type t = {identifier: string}; | ||
}; | ||
|
||
module Workspace = { | ||
[@deriving (show({with_path: false}), yojson({strict: false, exn: true}))] | ||
type t = {__test: string}; | ||
}; | ||
|
||
module Environment = { | ||
[@deriving (show({with_path: false}), yojson({strict: false, exn: true}))] | ||
type t = {globalStorageHomePath: string}; | ||
|
||
let create = (~globalStorageHomePath=Filesystem.unsafeFindHome(), ()) => { | ||
let ret: t = {globalStorageHomePath: globalStorageHomePath}; | ||
ret; | ||
}; | ||
}; | ||
|
||
[@deriving (show({with_path: false}), yojson({strict: false, exn: true}))] | ||
type t = { | ||
extensions: list(ExtensionInfo.t), | ||
parentPid: int, | ||
environment: Environment.t, | ||
logsLocationPath: string, | ||
autoStart: bool, | ||
}; | ||
|
||
let create = | ||
( | ||
~parentPid=Unix.getpid(), | ||
~extensions=[], | ||
~environment=Environment.create(), | ||
~logsLocationPath=Filesystem.unsafeFindHome(), | ||
~autoStart=true, | ||
(), | ||
) => { | ||
parentPid, | ||
extensions, | ||
environment, | ||
logsLocationPath, | ||
autoStart, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
/* | ||
* ExtensionHostProtocol.re | ||
* | ||
* This module documents & types the protocol for communicating with the VSCode Extension Host. | ||
* | ||
*/ | ||
|
||
module MessageType = { | ||
let initialized = 0; | ||
let ready = 1; | ||
let initData = 2; | ||
let terminate = 3; | ||
let requestJsonArgs = 4; | ||
}; | ||
|
||
module LogLevel = { | ||
let trace = 0; | ||
let debug = 1; | ||
let info = 2; | ||
let warning = 3; | ||
let error = 4; | ||
let critical = 5; | ||
let off = 6; | ||
}; | ||
|
||
module Environment = { | ||
type t = { | ||
/* isExtensionDevelopmentDebug: bool, */ | ||
/* appRootPath: string, */ | ||
/* appSettingsHomePath: string, */ | ||
/* extensionDevelopmentLocationPath: string, */ | ||
/* extensionTestsLocationPath: string, */ | ||
globalStorageHomePath: string, | ||
}; | ||
|
||
let create = (~globalStorageHomePath, ()) => { | ||
globalStorageHomePath; | ||
}; | ||
}; | ||
|
||
module Notification = { | ||
exception NotificationParseException(string); | ||
|
||
type t = { | ||
msgType: int, | ||
reqId: int, | ||
payload: Yojson.Safe.json, | ||
}; | ||
|
||
let of_yojson = (json: Yojson.Safe.json) => { | ||
switch (json) { | ||
| `Assoc([ | ||
("type", `Int(t)), | ||
("reqId", `Int(reqId)), | ||
("payload", payload), | ||
]) => { | ||
msgType: t, | ||
reqId, | ||
payload, | ||
} | ||
| _ => | ||
raise( | ||
NotificationParseException( | ||
"Unable to parse: " ++ Yojson.Safe.to_string(json), | ||
), | ||
) | ||
}; | ||
}; | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,8 @@ | |
(libraries | ||
yojson | ||
Oni_Core_Test | ||
Oni_Model_Test | ||
Oni_Extensions_Test | ||
Oni_Model_Test | ||
Oni_Neovim_Test | ||
)) | ||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@bryphe unsafe find home can cause a runtime error, I hide it to prevent its usage an alternative which is safer is to use a wrapped version that returns an
Error
orOk
like the rest ofFilesystem
just a heads up 👍 . An alternative is to usegetHomeDirectory
which returnsThat way this function has less of a chance to fail at runtime and kill the app for a user
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.
It doesn't require any
infix
operators or anything if that's a concernThere 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.
Thanks for the feedback and looking over the PR, @Akin909 !
What should we do in the case where we don't have a home directory (path for logs or storage) for the extension host client?
If we use the
result
object, it's not clear to me how we'd handle the case where we returnError
anyway, so I think having the exception (at least, its on startup) is reasonable - so we can catch it right away and fix it. This seems like a 'fail fast' case to me.Do you have an idea for alternative behavior?
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.
@bryphe my thinking was related to a user's workflow say I opened oni2 on a foreign system and for some strange reason we couldn't derive the home location, but I don't actually need the extension host I just want to view a file or do some other minor piece of work. I was thinking in such cases we might want to avoid an out right failure to boot I now have to debug, is it possible in this case that if we return an error we just shutdown or stop our attempts to use the host?
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.
maybe the
Environment.create
could returnSome
orNone
. If None we just return unit somewhere rather than carry on any further or if we had an error record in state we could dispatch an error action so we could eventually render it somewhere so the user is aware that things have gone wrong and some stuff won't be workingThere 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.
Wouldn't this be a bug that we should fix? Shouldn't there always be a
HOME
location? We have lots of features that 'depend' on aHOME
location - and we'll certainly have more as we support installing plugins, etc.Two downsides I see with the proposal:
First - we'd be running oni2 in a degraded mode - sure, we might sidestep the first bug, but then we get bugs saying syntax highlighting doesn't work, or config doesn't load, or language extensions don't work, and that actually takes more time to debug than the 'fail fast' scenario. We end up with bugs that manifest further from the source.
Second - it propagates complexity down to other features - every feature that touches the home directory has to handle the case where it doesn't exist:
Wouldn't it be simpler just to establish the invariant that we have a home directory and downstream features can rely on that? If we can't establish that invariant - we fail fast, fix the bug, and maintain that invariant. We sidestep this extra complexity of features that use the home directory having to figure out alternate behavior if it doesn't exist. As a maintainer - that's appealing to me!
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.
That's a fair point I can't see how this would fail in a way we could tolerate without a lot of other things breaking along the way, this case might be an exception (no pun intended).
It might be that the best thing is to completely fail but at the moment a runtime exception means the app closes abruptly with no indication.
I guess it's a larger issue than this specific function, I think failing fast is a good strategy.
But it would be good to consider these points as future places where we ought to pop up a dialog with some information then close the app etc.
maybe intercept the exception warn the user then crash 🤷♂️ (just my two cents, not wanting/trying to be contrarian)
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.
😆
One thing I didn't call it out earlier is its important to differentiate between a crash like this on startup vs at runtime - runtime is so much worse, because it could mean the user lost data.
Yes - I think what we could do for now, is add a set of 'pre-flight' checks when we start the app. For now, it might be that the app just quits and shows an error. But once we have a 'Welcome screen' for #81 - that would be an even better place to show an error message, and we could have a nicer experience.
Thanks for the feedback and thoughts, @Akin909 !
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.
Logged #193 to track the 'pre-flight' checks