Skip to content

Add PromiseHook bindings #453

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bnoordhuis
Copy link

@bnoordhuis bnoordhuis marked this pull request as draft April 22, 2025 08:41
@nabetti1720
Copy link

@bnoordhuis The preliminary check results are commented in the following places:
awslabs/llrt#947

@bnoordhuis bnoordhuis marked this pull request as ready for review April 25, 2025 19:23
Copy link

@nabetti1720 nabetti1720 left a comment

Choose a reason for hiding this comment

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

@bnoordhuis Thank you! I just made one comment.

@DelSkayn We are currently building an experimental runtime using this awesome Rust Binding and are looking forward to using these V8 inspired promise hooks in the runtime. Can you please review this PR when you have time?

@nabetti1720
Copy link

@DelSkayn PTAL, thanks in advance :)

Copy link
Owner

@DelSkayn DelSkayn left a comment

Choose a reason for hiding this comment

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

Looks good, only the bindings for other platforms need to be updated.

Let me know if you can access the artifacts from the CI action otherwise I will do the binding update in a separate PR which you can merge in.

@nabetti1720
Copy link

nabetti1720 commented Apr 30, 2025

Looks good, only the bindings for other platforms need to be updated.

Let me know if you can access the artifacts from the CI action otherwise I will do the binding update in a separate PR which you can merge in.

@DelSkayn Thank you! By the way, does the CI deliverable include aarch64/darwin?

When the owner of this PR asked me to test it, the bindings for my environment were not included, so I forked the branch from which the PR originated and added the bindings for aarch64/darwin to it for testing.
https://github.com/nabetti1720/bnoordhuis-rquickjs/tree/promisehook-aarch64

It's working fine so far, so I think you can go ahead and do it.

The status of the verification can be found here.
awslabs/llrt#947

@nabetti1720
Copy link

@DelSkayn Sorry for the confusing explanation. In my environment, I can't verify from CI artifacts, so it would be helpful if you could merge it once and then create another PR to update the binding. :)

@bnoordhuis
Copy link
Author

@DelSkayn I've updated the bindings except for x86_64-pc-windows-msvc because those two buildbots error while generating the bindings (i.e., before uploading them) for reasons that aren't clear to me.

@nabetti1720
Copy link

@DelSkayn I would be very grateful if you could check it out when you have time. :)

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