fix(extension/#2676): Part 1 - Fix error message with missing extensi…

…on dependency (#3007) __Issue:__ When an extension has a required dependency that is not installed, Onivim would fail to activate the extension silently. __Defect:__ Onivim was not properly handling the extension activation error - usually, it's a `string`, but in the case of a missing dependency, it is a JSON object - the parser was only handling the string case. __Fix:__ Implement a decoder to handle either case. Bubble up the activation error to the user. Related #2676 Related #1058
onivim · Jan 19, 2021 · 81d162b · 81d162b
1 parent 9e6a667
commit 81d162b
Show file tree

Hide file tree

Showing 7 changed files with 147 additions and 82 deletions.
diff --git a/CHANGES_CURRENT.md b/CHANGES_CURRENT.md
@@ -3,6 +3,7 @@
 ### Bug Fixes
 
 - #3008 - SCM: Fix index-out-of-bound exception when rendering diff markers
+- #3007 - Extensions: Show 'missing dependency' activation error to user
 
 ### Performance
 

diff --git a/src/Exthost/ExtensionActivationError.re b/src/Exthost/ExtensionActivationError.re
@@ -0,0 +1,32 @@
+open Oni_Core;
+
+[@deriving show]
+type t =
+  | Message(string)
+  | MissingDependency(ExtensionId.t);
+
+let toString =
+  fun
+  | Message(str) => str
+  | MissingDependency(extensionId) =>
+    Printf.sprintf(
+      "Missing required extension: %s",
+      ExtensionId.toString(extensionId),
+    );
+
+module Decode = {
+  open Json.Decode;
+  let message = string |> map(str => Message(str));
+
+  let missingDependency =
+    obj(({field, _}) => {field.required("dependency", ExtensionId.decode)})
+    |> map(extensionId => MissingDependency(extensionId));
+
+  let decode =
+    one_of([
+      ("message", message),
+      ("missingDependency", missingDependency),
+    ]);
+};
+
+let decode = Decode.decode;
diff --git a/src/Exthost/ExtensionId.re b/src/Exthost/ExtensionId.re
@@ -15,3 +15,5 @@ let decode =
   );
 
 let encode = Json.Encode.string;
+
+let toString = Fun.id;
diff --git a/src/Exthost/Exthost.re b/src/Exthost/Exthost.re
@@ -19,6 +19,7 @@ module DocumentSelector = DocumentSelector;
 module DocumentSymbol = DocumentSymbol;
 module Edit = Edit;
 module Eol = Eol;
+module ExtensionActivationError = ExtensionActivationError;
 module ExtensionActivationReason = ExtensionActivationReason;
 module ExtensionId = ExtensionId;
 module Files = Files;

diff --git a/src/Exthost/Exthost.rei b/src/Exthost/Exthost.rei
@@ -146,6 +146,15 @@ module Edit: {
   };
 };
 
+module ExtensionActivationError: {
+  [@deriving show]
+  type t =
+    | Message(string)
+    | MissingDependency(ExtensionId.t);
+
+  let toString: t => string;
+};
+
 module ExtensionActivationReason: {
   type t;
 
@@ -159,6 +168,8 @@ module ExtensionId: {
   [@deriving show]
   type t = string;
 
+  let toString: t => string;
+
   let decode: Json.decoder(t);
 };
 
@@ -174,17 +185,6 @@ module DefinitionLink: {
   let decode: Json.decoder(t);
 };
 
-// module DocumentFilter: {
-//   [@deriving show]
-//   type t;
-
-//   let matches: (~filetype: string, t) => bool;
-
-//   let decode: Json.decoder(t);
-
-//   let toString: t => string;
-// };
-
 module DocumentSelector: {
   [@deriving show]
   type t = list(DocumentFilter.t);
@@ -1220,12 +1220,14 @@ module Msg: {
           activateCallTime: int,
           activateResolvedTime: int,
         })
-      //activationEvent: option(string),
       | ExtensionActivationError({
           extensionId: ExtensionId.t,
-          errorMessage: string,
+          error: ExtensionActivationError.t,
         })
-      | ExtensionRuntimeError({extensionId: ExtensionId.t});
+      | ExtensionRuntimeError({
+          extensionId: ExtensionId.t,
+          errorsJson: list(Yojson.Safe.t),
+        });
   };
 
   module FileSystem: {

diff --git a/src/Exthost/Msg.re b/src/Exthost/Msg.re
@@ -538,76 +538,86 @@ module ExtensionService = {
         activateCallTime: int,
         activateResolvedTime: int,
       })
-    //activationEvent: option(string),
     | ExtensionActivationError({
         extensionId: ExtensionId.t,
-        errorMessage: string,
+        error: ExtensionActivationError.t,
       })
-    | ExtensionRuntimeError({extensionId: ExtensionId.t});
-  let withExtensionId = (f, extensionIdJson) => {
-    extensionIdJson
-    |> Json.Decode.decode_value(ExtensionId.decode)
-    |> Result.map(f)
-    |> Result.map_error(Json.Decode.string_of_error);
-  };
+    | ExtensionRuntimeError({
+        extensionId: ExtensionId.t,
+        errorsJson: list(Yojson.Safe.t),
+      });
 
   let handle = (method, args: Yojson.Safe.t) => {
-    switch (method, args) {
-    | ("$activateExtension", `List([extensionIdJson])) =>
-      extensionIdJson
-      |> withExtensionId(extensionId => {
-           ActivateExtension({extensionId, activationEvent: None})
-         })
-    | ("$activateExtension", `List([extensionIdJson, activationEventJson])) =>
-      let activationEvent =
-        switch (activationEventJson) {
-        | `String(v) => Some(v)
-        | _ => None
+    Base.Result.Let_syntax.(
+      {
+        switch (method, args) {
+        | ("$activateExtension", `List([extensionIdJson])) =>
+          let%bind extensionId =
+            extensionIdJson |> Internal.decode_value(ExtensionId.decode);
+          Ok(ActivateExtension({extensionId, activationEvent: None}));
+
+        | (
+            "$activateExtension",
+            `List([extensionIdJson, activationEventJson]),
+          ) =>
+          let activationEvent =
+            switch (activationEventJson) {
+            | `String(v) => Some(v)
+            | _ => None
+            };
+
+          let%bind extensionId =
+            extensionIdJson |> Internal.decode_value(ExtensionId.decode);
+          Ok(ActivateExtension({extensionId, activationEvent}));
+        | (
+            "$onExtensionActivationError",
+            `List([extensionIdJson, errorJson]),
+          ) =>
+          let%bind extensionId =
+            extensionIdJson |> Internal.decode_value(ExtensionId.decode);
+
+          let%bind error =
+            errorJson
+            |> Internal.decode_value(ExtensionActivationError.decode);
+
+          Ok(ExtensionActivationError({extensionId, error}));
+
+        | ("$onWillActivateExtension", `List([extensionIdJson])) =>
+          let%bind extensionId =
+            extensionIdJson |> Internal.decode_value(ExtensionId.decode);
+          Ok(WillActivateExtension({extensionId: extensionId}));
+        | (
+            "$onDidActivateExtension",
+            `List([
+              extensionIdJson,
+              `Int(codeLoadingTime),
+              `Int(activateCallTime),
+              `Int(activateResolvedTime),
+              ..._args,
+            ]),
+          ) =>
+          let%bind extensionId =
+            extensionIdJson |> Internal.decode_value(ExtensionId.decode);
+          Ok(
+            DidActivateExtension({
+              extensionId,
+              codeLoadingTime,
+              activateCallTime,
+              activateResolvedTime,
+            }),
+          );
+        | (
+            "$onExtensionRuntimeError",
+            `List([extensionIdJson, ...errorsJson]),
+          ) =>
+          let%bind extensionId =
+            extensionIdJson |> Internal.decode_value(ExtensionId.decode);
+          Ok(ExtensionRuntimeError({extensionId, errorsJson}));
+
+        | _ => Error("Unhandled method: " ++ method)
         };
-
-      extensionIdJson
-      |> withExtensionId(extensionId => {
-           ActivateExtension({extensionId, activationEvent})
-         });
-    | (
-        "$onExtensionActivationError",
-        `List([extensionIdJson, `String(errorMessage)]),
-      ) =>
-      extensionIdJson
-      |> withExtensionId(extensionId => {
-           ExtensionActivationError({extensionId, errorMessage})
-         })
-    | ("$onWillActivateExtension", `List([extensionIdJson])) =>
-      extensionIdJson
-      |> withExtensionId(extensionId => {
-           WillActivateExtension({extensionId: extensionId})
-         })
-    | (
-        "$onDidActivateExtension",
-        `List([
-          extensionIdJson,
-          `Int(codeLoadingTime),
-          `Int(activateCallTime),
-          `Int(activateResolvedTime),
-          ..._args,
-        ]),
-      ) =>
-      extensionIdJson
-      |> withExtensionId(extensionId => {
-           DidActivateExtension({
-             extensionId,
-             codeLoadingTime,
-             activateCallTime,
-             activateResolvedTime,
-           })
-         })
-    | ("$onExtensionRuntimeError", `List([extensionIdJson, ..._args])) =>
-      extensionIdJson
-      |> withExtensionId(extensionId => {
-           ExtensionRuntimeError({extensionId: extensionId})
-         })
-    | _ => Error("Unhandled method: " ++ method)
-    };
+      }
+    );
   };
 };
 

diff --git a/src/Feature/Extensions/Model.re b/src/Feature/Extensions/Model.re
@@ -608,15 +608,32 @@ let getExtensions = Internal.getExtensions;
 
 let update = (~extHostClient, msg, model) => {
   switch (msg) {
-  | Exthost(WillActivateExtension(_))
-  | Exthost(ExtensionRuntimeError(_)) => (model, Nothing)
+  | Exthost(WillActivateExtension(_)) => (model, Nothing)
+
+  | Exthost(ExtensionRuntimeError({extensionId, errorsJson})) => (
+      model,
+      NotifyFailure(
+        Printf.sprintf(
+          "Extension runtime error %s:%s",
+          Exthost.ExtensionId.toString(extensionId),
+          Yojson.Safe.to_string(`List(errorsJson)),
+        ),
+      ),
+    )
+
   | Exthost(ActivateExtension({extensionId, _})) => (
       Internal.markActivated(extensionId, model),
       Nothing,
     )
-  | Exthost(ExtensionActivationError({errorMessage, _})) => (
+  | Exthost(ExtensionActivationError({error, extensionId})) => (
       model,
-      NotifyFailure(Printf.sprintf("Error: %s", errorMessage)),
+      NotifyFailure(
+        Printf.sprintf(
+          "Error activating extension %s: %s",
+          Exthost.ExtensionId.toString(extensionId),
+          Exthost.ExtensionActivationError.toString(error),
+        ),
+      ),
     )
   | Exthost(DidActivateExtension({extensionId, _})) => (
       Internal.markActivated(extensionId, model),
Original file line number	Diff line number	Diff line change
Expand Up		@@ -15,3 +15,5 @@ let decode =
		);

		let encode = Json.Encode.string;

		let toString = Fun.id;