Skip to content
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

Merged
merged 27 commits into from
Sep 21, 2022

Conversation

comcalvi
Copy link
Contributor

@comcalvi comcalvi commented Sep 8, 2022

Adds the JsiiFault and JsError 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.

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 8, 2022
@comcalvi
Copy link
Contributor Author

comcalvi commented Sep 8, 2022

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.

@comcalvi comcalvi marked this pull request as draft September 8, 2022 22:52
@MrArnoldPalmer
Copy link
Contributor

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.

@comcalvi comcalvi changed the title feat(kernel): add JsiiFault error type feat(kernel): classify JsiiFaults and JsErrors Sep 13, 2022
@comcalvi comcalvi changed the title feat(kernel): classify JsiiFaults and JsErrors feat(kernel): distinguish framework errors from userland errors Sep 14, 2022
@comcalvi comcalvi changed the title feat(kernel): distinguish framework errors from userland errors feat(kernel): distinguish framework errors from userland errors in Java Sep 14, 2022
@comcalvi comcalvi marked this pull request as ready for review September 14, 2022 21:41
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a 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 👍🏻

@comcalvi comcalvi added the pr/do-not-merge This PR should not be merged at this time. label Sep 15, 2022
/*
* Represents an error thrown from the user JS library.
*/
public final class JSException extends JsiiBaseException {
Copy link
Contributor

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).

Copy link
Contributor Author

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?

Copy link
Contributor

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/kernel/src/kernel.ts Outdated Show resolved Hide resolved
…ents the fault. JSException is now java.lang.RuntimeException.
Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there :)

@comcalvi comcalvi removed the pr/do-not-merge This PR should not be merged at this time. label Sep 21, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 21, 2022

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Sep 21, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 21, 2022

Merging (with squash)...

@mergify
Copy link
Contributor

mergify bot commented Sep 21, 2022

Merging (with squash)...

@mergify mergify bot merged commit a4d39c6 into aws:main Sep 21, 2022
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Sep 21, 2022
MrArnoldPalmer added a commit that referenced this pull request Sep 22, 2022
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
mergify bot pushed a commit that referenced this pull request Sep 24, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants