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

feat: allow running JavaScript workers with async methods #143

Merged
merged 2 commits into from
May 29, 2023

Conversation

Angelmmiguel
Copy link
Contributor

Add support for async methods in JavaScript workers. For that, I split the JavaScript runtime in two steps:

  • Call the requestToHandler method to get the response method / object.
  • Convert response to a Promise so we cover all use cases (async / sync).
  • Save the final response in a new result variable.

To add support on the JS Runtime side, I upgraded the JS kit to use the new javy crate. It adds new features and a better high-level APIs for running JS code in Rust.

I also included a new example and a new e2e test.

It closes #142

@Angelmmiguel Angelmmiguel added the 🚀 enhancement New feature or request label May 29, 2023
@Angelmmiguel Angelmmiguel added this to the v1.2.0 milestone May 29, 2023
@Angelmmiguel Angelmmiguel requested a review from a team May 29, 2023 09:39
@Angelmmiguel Angelmmiguel self-assigned this May 29, 2023
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

LGTM 🎉; this is great!, although I think it would be good to split the main commit into two:

  • quickjs -> javy, binary removal
  • async feature

@Angelmmiguel
Copy link
Contributor Author

LGTM 🎉; this is great!, although I think it would be good to split the main commit into two:

  • quickjs -> javy, binary removal
  • async feature

Sure, let me do it for traceability on the PR, although they will be squashed on main.

@Angelmmiguel Angelmmiguel force-pushed the 142-support-async-js-workers branch from 63d10dc to f050856 Compare May 29, 2023 14:27
@Angelmmiguel
Copy link
Contributor Author

Done @ereslibre ! 😄

@ereslibre
Copy link
Contributor

👏 Could we merge without squash? I think we can rebase and merge, no?

If so, that would be great! Thanks a lot @Angelmmiguel.

@Angelmmiguel
Copy link
Contributor Author

👏 Could we merge without squash? I think we can rebase and merge, no?

If so, that would be great! Thanks a lot @Angelmmiguel.

Yeah, that's possible too. However, I usually converge commits into a single one that refers to the PR. Sometimes PRs may include multiple commits like fix: fmt and fix: typo. Instead of asking people to rewrite the git history, I believe it's simpler to limit the PR scope and squash it.

@ereslibre
Copy link
Contributor

ereslibre commented May 29, 2023

Instead of asking people to rewrite the git history, I believe it's simpler to limit the PR scope and squash it.

👍, however if I'm not mistaken the GitHub option "Rebase and merge" does not enforce anybody to rebase their own history, given they will just fast forward on top of these changes.

Anyhow, it's fine by me.

@Angelmmiguel
Copy link
Contributor Author

Instead of asking people to rewrite the git history, I believe it's simpler to limit the PR scope and squash it.

👍, however if I'm not mistaken the GitHub option "Rebase and merge" does not enforce anybody to rebase their own history, given they will just fast forward on top of these changes.

Anyhow, it's fine by me.

Yeah, that option is available. I usually squash all PRs, but I agree it's interesting to keep the commits separated in main for this specific one. I will rebase and merge the two commits in this one 😄

@Angelmmiguel Angelmmiguel merged commit ec2fc6f into main May 29, 2023
@Angelmmiguel Angelmmiguel deleted the 142-support-async-js-workers branch June 30, 2023 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support async methods in the JavaScript kit
2 participants