Skip to content

allow zig js host functions to return JSError - #15120

Merged
nektro merged 9 commits into
mainfrom
nektro-patch-56942
Nov 14, 2024
Merged

allow zig js host functions to return JSError#15120
nektro merged 9 commits into
mainfrom
nektro-patch-56942

Conversation

@nektro

@nektro nektro commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

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

@robobun

robobun commented Nov 13, 2024

Copy link
Copy Markdown
Collaborator

@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

    @paperclover paperclover left a comment

    Copy link
    Copy Markdown
    Contributor

    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

    nektro commented Nov 13, 2024

    Copy link
    Copy Markdown
    Contributor Author

    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

    @paperclover

    Copy link
    Copy Markdown
    Contributor

    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

    nektro commented Nov 13, 2024

    Copy link
    Copy Markdown
    Contributor Author

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

    Comment thread src/bun.zig
    /// You should almost never have to construct this manually.
    JSError,
    // XXX: This is temporary! meghan will remove this soon
    OutOfMemory,

    Copy link
    Copy Markdown
    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
    Copy Markdown
    Contributor 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 requested a review from dylan-conway November 14, 2024 04:58
    @nektro
    nektro merged commit fdd8d35 into main Nov 14, 2024
    @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