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

allow zig js host functions to return JSError #15120

Merged
merged 9 commits into from
Nov 14, 2024
Merged

allow zig js host functions to return JSError #15120

merged 9 commits into from
Nov 14, 2024

Conversation

nektro
Copy link
Member

@nektro nektro commented Nov 13, 2024

progress towards zig JSValue stability and safety in #159
more followups to come

@robobun
Copy link

robobun commented Nov 13, 2024

@nektro, your commit 891daa2 has 3 failures in #6080:

  • test/js/bun/http/serve.test.ts - 1 failing on 🍎 x64
  • test/js/web/fetch/fetch.tls.test.ts - 1 failing on 🍎 aarch64
  • test/regression/issue/ctrl-c.test.ts - 1 failing on 🍎 aarch64
  • @nektro nektro marked this pull request as ready for review November 13, 2024 02:52
    Copy link
    Member

    @paperdave paperdave left a comment

    Choose a reason for hiding this comment

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

    should we actually be annotating every function with bun.JSError!? i think my solution to this in dave/more-bake is better, since it doesnt include the catch control flow on functions that don't return an error

    @nektro
    Copy link
    Member Author

    nektro commented Nov 13, 2024

    nearly every function annotated will be taking advantage of it eventually and unless there's a noticeable perf effect then the consistency makes the function's purpose very clear at a glance

    a big motivator for this project is to make the code interacting with these values as predictable as possible to interact with so that it becomes hard or impossible to introduce certain kinds of bugs

    @paperdave
    Copy link
    Member

    if we want all of these functions to have the explicit annotation, it should be bun.OOM || bun.JSError, which i have in my branch defined as bun.JSOOM (can be named something else). the wrapper checks and handles both of these errors, if specified.

    this is why i asked to wait a day for my branch to finish, as i'm nearly close to merging it. was this blocking something else?

    @nektro
    Copy link
    Member Author

    nektro commented Nov 13, 2024

    in a couple more stages i think it would make more sense for OutOfMemory to be a part of bun.JSError, without a union
    pr is only blocking itself

    edit: in a few more after that global.throw* will return JSError and we can use global.throwOutOfMemory()

    JSError,
    // XXX: This is temporary! meghan will remove this soon
    OutOfMemory,
    Copy link
    Member

    Choose a reason for hiding this comment

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

    I think we should consider keeping this here and rename error.JSError to error.ThrownException or something similar

    Copy link
    Member Author

    Choose a reason for hiding this comment

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

    yeah there's many followups ahead of this

    @nektro nektro merged commit fdd8d35 into main Nov 14, 2024
    22 of 25 checks passed
    @nektro nektro deleted the nektro-patch-56942 branch November 14, 2024 05:11
    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.

    4 participants