-
Notifications
You must be signed in to change notification settings - Fork 245
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
feat(kernel): distinguish framework errors from userland errors in Java #3747
Conversation
In order to surface any of this to the user, we need to pass the error type over the wire to the runtime. It's not clear where exactly that happens. |
This is where that happens basically https://github.com/aws/jsii/pull/3747/files#diff-0c5a3339fc4062c6aadbff20267f2a7bd57f04149e33984a6478e7664eaf0f4eR182 Write error is what serializes the error object before being passed to the stdout stream for writing. |
…ethod can't distinguish them yet
…ach, needs to be fixed before this can be merged
JsiiFault
error typeJsiiFault
s and JsError
s
JsiiFault
s and JsError
sThere 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.
Couple small things, otherwise straightforward enough and easy to iterate on for further functionality 👍🏻
packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsException.java
Outdated
Show resolved
Hide resolved
/* | ||
* Represents an error thrown from the user JS library. | ||
*/ | ||
public final class JSException extends JsiiBaseException { |
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.
Prefer avoiding abbreviations where possible... Can this instead be JavascriptException
?
Also - Java exceptions are Serializable
so they should carry a public static final long serialVersionUID
property (can start at 1L
).
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.
this will be inherited from the base class though, no?
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.
Statics are not inherited nor inheritable. Java allows you to refer to a static declared on a parent type though a child type, but this is actually not inheritance (and best practices encourage you to avoid doing that, precisely because the semantics might change if the child class adds a new declaration, shadowing -- and not overriding -- the one from the parent class).
packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiException.java
Show resolved
Hide resolved
…ents the fault. JSException is now java.lang.RuntimeException.
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.
Almost there :)
...sii/java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java
Outdated
Show resolved
Hide resolved
packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiError.java
Outdated
Show resolved
Hide resolved
packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiException.java
Outdated
Show resolved
Hide resolved
packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java
Outdated
Show resolved
Hide resolved
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
Merging (with squash)... |
Merging (with squash)... |
Adds a new class `JsiiError` that subclasses `JsiiException` to help distinguish jsii kernel errors that are likely unrecoverable from `RuntimeErrors` which may be errors expected and handled within the JS process and may be caught and handled in the host language runtime. See #3747 for more information
Adds a new class `JsiiError` that subclasses `JsiiException` to help distinguish jsii kernel errors that are likely unrecoverable from `RuntimeErrors` which may be errors expected and handled within the JS process and may be caught and handled in the host language runtime. See #3747 for more information --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
Adds the
JsiiFault
andJsError
types to the Kernel and corresponding types in Java. This will provide a better error experience and make it clearer which errors come from the jsii framework itself (eg a broken pipe) and which errors from the user code (eg a CDK construct validation error).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.