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

Improve error messages when JS code throws exceptions #173

Conversation

pedrovgs
Copy link
Contributor

We've noticed when JS code throws an exception evaluated from JSFunction the error reported shows this message:

RuntimeError: Unreachable code should not be executed (evaluating 'this.exports.swjs_call_host_function(e,a,r,u)')
  [native code] at line 0
   ../node_modules/javascript-kit-swift/Runtime/lib/index.mjs at line 348

After reviewing this with the team, @kateinoigakukun suggested there where a try/catch capturing the exception we should remove in order to improve this scenario.

This PR adds two new unsafe versions of already existing functions where the JS side of the project doesn't capture any exception so the final result is a better error reporting like this:

Screen_Shot_2022-03-30_at_10 35 52

…can improve error reporting by not hiding the information we get form the JS crash
@pedrovgs
Copy link
Contributor Author

Hey @kateinoigakukun thank you so much for your help. I applied the changes you suggested but I've found one test I can't fix related to JSClosure. test("Closure Lifetime") seems to be failing but I don't get what's going on. Could you help me with this? Thank you in advance 😃

Sources/JavaScriptKit/FundamentalObjects/JSFunction.swift Outdated Show resolved Hide resolved
Runtime/src/index.ts Outdated Show resolved Hide resolved
…nction_with_this and _call_function instead of the unsafe version we created
@pedrovgs
Copy link
Contributor Author

Thanks for the tip @j-f1 I didn't notice the code was being invoked from JSThrowingFunction. I've added a new commit with the suggested change 😃

…ption

Co-authored-by: Jed Fox <git@jedfox.com>
@pedrovgs pedrovgs marked this pull request as ready for review March 30, 2022 15:54
Runtime/src/index.ts Outdated Show resolved Hide resolved
@pedrovgs
Copy link
Contributor Author

@j-f1 @kateinoigakukun thank you all for your comments and your review. I think I've applied all your suggestions. Please let me know if you want me to review any other detail or change anything 😃 Thanks!

Copy link
Member

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

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

Sorry, one more thing. Could you bump up the interface version in

@pedrovgs
Copy link
Contributor Author

pedrovgs commented Mar 31, 2022

No need to be sorry @kateinoigakukun 😃 Version value updated from 705 to 706!

@kateinoigakukun kateinoigakukun merged commit 6ae7311 into swiftwasm:main Mar 31, 2022
@kateinoigakukun
Copy link
Member

Thank you very much!

@pedrovgs pedrovgs deleted the do-not-catch-js-exceptions-in-js-function branch March 31, 2022 10:22
MaxDesiatov added a commit that referenced this pull request Mar 31, 2022
I'd be happy to wait a bit more to include DOMKit improvements, but #173 has been highly requested to be included in a release sooner rather than later. After all, this means that we have less pressure to rush DOMKit-related changes out in this release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants