Refactor JavaScript local exceptions #2574

pepone · 2024-07-25T18:07:08Z

Refactoring JavaScript local exceptions, similar to refactoring done in other language mappings.

pepone · 2024-07-25T18:08:52Z

js/src/Ice/Exception.js

@@ -2,87 +2,12 @@
 // Copyright (c) ZeroC, Inc. All rights reserved.
 //

-const toString = function (key, object, objectTable, ident) {


This was used to override the standard toString method and print all attributes. It seems overcomplicated and of little use.

That's not a concern for local exceptions, since they must all provide a message. But it could be a concern for user exceptions that never provide any message.

When you print (toString) a user exception, what do you get? Only the type ID?
In C++ and C#, we give users the ability to define a custom "toString" for their user exceptions. How does this work in JS?

Note that constructing a user exception with a message string is not a good solution: user exceptions are often unmarshaled and the unmarshaling code doesn't have a message string to provide.

In JS you can patch the class prototype to override a method. This would allow users to provide their own toString

pepone · 2024-07-25T18:19:27Z

js/src/Ice/Exception.js

-        return s;
-    }
-
-    static captureStackTrace(object) {


We don't need this. The stack property is already part of the base Error class.

See https://playcode.io/1950124

pepone · 2024-07-25T18:20:05Z

js/src/Ice/IdentityToString.js

+ *
+ * @return The string representation of the object identity.
+ **/
+export function identityToString(ident, toStringMode = ToStringMode.Unicode) {


Move to a separate file to avoid circular dependencies with LocalExceptions.js

pepone · 2024-07-25T18:21:59Z

js/src/Ice/TraceUtil.js

+    // s.push("\nfacet = ");
+    // if (facet.length > 0) {
+    //    s.push(StringUtil.escapeString(facet[0], "", toStringMode));
+    // }


This code was broken, and when trying to fix it I noticed this introduces a circular dependency. I will fix it in a separate PR.

bernardnormier

Looks good.

bernardnormier · 2024-07-25T20:01:58Z

js/src/Ice/ExUtil.js


 export function throwUOE(expectedType, v) {
+    // If the object is an unknown sliced object, we didn't find an value factory.


a value factory

bernardnormier · 2024-07-25T20:03:21Z

js/src/Ice/ExUtil.js

-        "expected element of type `" + expectedType + "' but received `" + type + "'",
-        type,
-        expectedType,
+    let expected = expectedType.ice_staticId(); // TODO add ice_staticId


expectedType should be a type ID to start with.

bernardnormier · 2024-07-25T20:05:15Z

js/test/Ice/exceptions/Client.ts

@@ -55,14 +55,14 @@ export class Client extends TestHelper {
                adapter.add(new EmptyI(), Ice.stringToIdentity(""));
                test(false);
            } catch (ex) {
-                test(ex instanceof Ice.IllegalIdentityException, ex);
+                test(ex instanceof Ice.LocalException, ex);


We don't get a more specific exception?

Maybe we can use TypeError https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypeError

bernardnormier · 2024-07-25T20:11:21Z

js/src/Ice/UserException.js

@@ -5,11 +5,6 @@
 import { Exception } from "./Exception.js";

 export class UserException extends Exception {
-    constructor() {


Instead of inheriting Exception's ctor, I think UserException should define a () constructor. Derived user exceptions should not support setting the error message (and more) at construction time.

bernardnormier · 2024-07-25T20:16:43Z

js/src/Ice/LocalException.js

-
-    static get _id() {
-        return "::Ice::LocalException";
+    constructor(message, cause) {


I lean toward simply inheriting the constructor. In particular, when you throw a derived local exception with a cause, we should use the JS error syntax:

throw new MarshalException("Message", { cause: err });

Providing another syntax sounds confusing.

bernardnormier · 2024-07-25T20:17:34Z

js/src/Ice/LocalExceptions.js

-    constructor(kindOfObject = "", id = "", _cause = "") {
-        super(_cause);
-        this.kindOfObject = kindOfObject;
+    constructor(typeName = "", id = new Ice.Identity(), facet = "", operation = "") {


typeName should not be defaulted.

bernardnormier · 2024-07-25T20:20:09Z

js/src/Ice/LocalExceptions.js

    }

-    static get _id() {
-        return "::Ice::EndpointSelectionTypeParseException";
+    constructor(message) {


Use inherit ctor.

bernardnormier · 2024-07-25T20:20:42Z

js/src/Ice/LocalExceptions.js

    }

-    static get _id() {
-        return "::Ice::VersionParseException";
+    constructor(message) {


bernardnormier · 2024-07-25T20:27:15Z

js/src/Ice/LocalExceptions.js

-export class CompressionException extends ProtocolException {
-    constructor(reason, _cause = "") {
-        super(reason, _cause);
+    constructor(strOrProxy) {


messageOrProxy would be clearer.

bernardnormier · 2024-07-25T20:28:51Z

js/src/Ice/LocalExceptions.d.ts

-            constructor(reason?: string, ice_cause?: string | Error);
-            reason: string;
+        class RequestFailedException extends LocalException {
+            constructor(typeName?: string, id?: Identity, facet?: string, operation?: string);


typeName should not be optional

bernardnormier · 2024-07-26T13:39:42Z

js/test/Ice/servantLocator/ServantLocatorI.ts

@@ -82,7 +82,7 @@ export class ServantLocatorI implements Ice.ServantLocator {
        if (current.operation == "ice_ids") {
            throw new Test.TestIntfUserException();
        } else if (current.operation == "requestFailedException") {
-            throw new Ice.ObjectNotExistException();
+            throw new Ice.ObjectNotExistException(current.id, current.facet, current.operation);


It's not clear to me why you don't use the default ctor here.

In general, the preferred/recommended way to throw *NotExistException from application code is without arguments.

I need to fix the default ctor to not generate a message then, it doesn't work with the refactored code.

…eroc-ice/ice#2574)

Refactor JavaScript local exceptions

61a28df

pepone requested review from externl and bernardnormier July 25, 2024 18:07

pepone commented Jul 25, 2024

View reviewed changes

Set the Error.name property

e1c9826

pepone commented Jul 25, 2024

View reviewed changes

bernardnormier approved these changes Jul 25, 2024

View reviewed changes

Review fixes

9d661f7

bernardnormier self-requested a review July 26, 2024 13:34

bernardnormier approved these changes Jul 26, 2024

View reviewed changes

externl approved these changes Jul 26, 2024

View reviewed changes

pepone added 2 commits July 26, 2024 16:05

Review fixes

b32cb23

Fix test failure

83365aa

pepone merged commit 806f28c into zeroc-ice:main Jul 26, 2024
7 checks passed

pepone mentioned this pull request Aug 2, 2024

Simplify user exception mapping #2527

Closed

9 tasks

InsertCreativityHere pushed a commit to InsertCreativityHere/compiler-comparison that referenced this pull request Jan 1, 2025

HEAD:(zeroc-ice/ice@806f28c7f5) Refactor JavaScript local exceptions (z…

97a5206

…eroc-ice/ice#2574)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor JavaScript local exceptions #2574

Refactor JavaScript local exceptions #2574

pepone commented Jul 25, 2024

pepone Jul 25, 2024

bernardnormier Jul 25, 2024 •

edited

Loading

pepone Jul 25, 2024

pepone Jul 25, 2024

pepone Jul 25, 2024

pepone Jul 25, 2024

bernardnormier left a comment

bernardnormier Jul 25, 2024

bernardnormier Jul 25, 2024

bernardnormier Jul 25, 2024

pepone Jul 25, 2024

bernardnormier Jul 25, 2024

bernardnormier Jul 25, 2024

bernardnormier Jul 25, 2024

bernardnormier Jul 25, 2024

bernardnormier Jul 25, 2024

bernardnormier Jul 25, 2024

bernardnormier Jul 25, 2024

bernardnormier Jul 26, 2024

pepone Jul 26, 2024


		export function throwUOE(expectedType, v) {
		// If the object is an unknown sliced object, we didn't find an value factory.

Refactor JavaScript local exceptions #2574

Refactor JavaScript local exceptions #2574

Conversation

pepone commented Jul 25, 2024

Choose a reason for hiding this comment

bernardnormier Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier Jul 25, 2024 •

edited

Loading