Extension Host: Send initialization data #189
Conversation
| @@ -28,6 +28,8 @@ let mkdir: (string, ~perm: int=?, unit) => t(unit); | |||
|
|
|||
| let rmdir: string => t(unit); | |||
|
|
|||
| let unsafeFindHome: unit => string; | |||
There was a problem hiding this comment.
@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 or Ok like the rest of Filesystem just a heads up 👍 . An alternative is to use getHomeDirectory which returns
type t('a) = {
| Error(string),
| Ok(a)
}That 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.
It doesn't require any infix operators or anything if that's a concern
There was a problem hiding this comment.
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 return Error 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.
@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.
maybe the Environment.create could return Some or None. 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 working
There was a problem hiding this comment.
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,
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 a HOME 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:
let openConfigurationFile = (effects: Effects.t, name) =>
switch (Filesystem.createOniConfigFile(name)) {
| Ok(path) => effects.openFile(~path, ())
| Error(e) => print_endline(e)
};
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.
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.
switch (result)
| Error(e) => {
Revery.modal.show("Something bad happened" ++ e);
failwith("Intolerable Exception")
| Ok(value) => carryOn(value)
}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.
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.
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.
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.
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.
Logged #193 to track the 'pre-flight' checks
bryphe
changed the title
|
@Akin909 , I hope you don't mind, I'll bring this in now since I have a few other extension host items queued up - but we can continue thinking about the error / fail-fast ideas in #193 . Thanks for reviewing the change! |
* Use new Rench API for pid * Use corrected pid function from Rench * Add regression test to validate we're sending the right PID * Formatting * Dependency: revery -> 0.15.3 (#191) * update package.json * Update lockfiles * Extension Host: Send initialization data (#189) * Initial integration * Revert vsync change * Start adding extension host client Reason API * Initial ExtensionHost client start-up * Get extension host running * Hook up entry point to process messages * Add basic test case to verify we get 'Ready' message from extension host * Formatting * Tweak runner * Factor protocol out * Fix ExtHostProtocol path * Formatting * Fix merge conflict * Start stubbing out InitDat * Get ExtensionHostInitData compiling * Formatting * Debugging exception on startup * Refactor interface, test for 'initialized' event * Formatting * Remove logging * bump
Right now, we have the 'extension host' running, but not much is happening - it sends us an 'Initialized' message in #178 . This is the start of the 'handshake' for the extension host protocol.
To complete the handshake, we need to send it an bundle of initialization data, with context about where the logs are, the parent process PID, workspace, etc - and then once we send that, the extension host will send us a 'ready' message and activate all the wildcard extensions.