search

awslabs / llrt

LLRT (Low Latency Runtime) is an experimental, lightweight JavaScript runtime designed to address the growing demand for fast and efficient Serverless applications.
Apache License 2.0
8.04k stars 355 forks source link

feat: :arrow_up: Upgrade to rquickjs 0.6.2 #377

Closed KaanMol closed 3 months ago

KaanMol commented 4 months ago

Issue # (if available)

Description of changes

Upgraded rquickjs from 0.5.1 to 0.6.2.

4 tests still seem to fail, I tried but could not figure out how to resolve them. I hope someone will directly notice the mistakes I made and guide me to fix them :)

Checklist

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

richarddavison commented 4 months ago

@KaanMol Thanks a lot for the PR. I've started working on this as soon as it came out :) I'm afraid this is a bit complex as TLA (Top Level Await) breaks the require functionality. In previous versions of QuickJS imports where always sync, now they return promises. This means that we need to drive promises, futures and async tasks simultaneously and a wait them from a sync function (require) which is currently not possible in latest version of rquickjs. I've submitted a PR for this in the binding lib: https://github.com/DelSkayn/rquickjs/pull/310. If that doesn't get merged, I'm considering using cargo-patch to expose the task spawner from rquickjs so I can work around this.

KaanMol commented 4 months ago

I

@KaanMol Thanks a lot for the PR. I've started working on this as soon as it came out :) I'm afraid this is a bit complex as TLA (Top Level Await) breaks the require functionality. In previous versions of QuickJS imports where always sync, now they return promises. This means that we need to drive promises, futures and async tasks simultaneously and a wait them from a sync function (require) which is currently not possible in latest version of rquickjs. I've submitted a PR for this in the binding lib: DelSkayn/rquickjs#310. If that doesn't get merged, I'm considering using cargo-patch to expose the task spawner from rquickjs so I can work around this.

I noticed that as well, I also saw the PR you have open, so once yours gets merged I was hoping to get it in immediately :). I am not expecting this PR to be merged very soon, but to already have it there :) There are some weird things happening I noticed indeed, make test worked initially and when I restarted my editor the test just don't run anymore, the Object to Promise change has a weird quirk somewhere I think

offtopic: Love the profilepic btw on your other account 🤣

richarddavison commented 3 months ago

@KaanMol thanks again for the PR! This took way longer than expected to bring in but it's now opened in https://github.com/awslabs/llrt/pull/429 so I'll close this one :)