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

Improve implementation of example JobQueues #4111

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Conversation

jedel1043
Copy link
Member

Fixes #3531.

I'm surprised tokio uses so few dependencies, but I'm not complaining.

@jedel1043 jedel1043 added enhancement New feature or request documentation update documentation labels Jan 7, 2025
@jedel1043 jedel1043 added this to the next-release milestone Jan 7, 2025
@jedel1043 jedel1043 requested a review from a team January 7, 2025 08:07
Copy link

github-actions bot commented Jan 7, 2025

Test262 conformance changes

Test result main count PR count difference
Total 50,254 50,254 0
Passed 44,995 44,995 0
Ignored 1,740 1,740 0
Failed 3,519 3,519 0
Panics 0 0 0
Conformance 89.54% 89.54% 0.00%

@hansl
Copy link
Contributor

hansl commented Jan 7, 2025

Aside: Should we put this code (and maybe others) to a boa_job_queues or similar so it's easier to replicate?

@jedel1043
Copy link
Member Author

@hansl based off of this reddit post, I'm not sure if people really like when libraries scatter an usage example into multiple files 😅

@hansl
Copy link
Contributor

hansl commented Jan 7, 2025

I understood this post for being "stop making examples thousands of lines long with implementations I might not need", not "stop abstracting reusable parts and using them in examples".

In my ideal world, I'd rather have this: https://github.com/bytecodealliance/wasmtime/blob/main/examples/gcd.rs than having to implement a job queue just for an example, which becomes bloated (multiple files, hundreds of lines, etc). If anything this PR adds exactly what the Reddit post was expressing as frustration.

Reusable pieces should be moved to the engine or a separate crate, and used in examples to show how simple adding Boa to your project is. Which is really what we want people to take away; add Boa, get JS running in 20 lines of code! Easy.

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

The discussion above is not blocking to the code going in.

@jedel1043
Copy link
Member Author

@hansl Ahh, I interpreted your comment as abstracting the job queue implementation for only the examples. I suppose you mean maintaining a crate of job queues for ease of use? I can see the utility in that. I would even put the implementations on boa_runtime, since I think it's pretty obvious that runtimes would want to run on a fully-fledged event loop.

@jedel1043
Copy link
Member Author

I wouldn't use that in the futures examples though, because people still would want to know how to implement their own event loop. We can add a separate example on how to use the runtime.

@jedel1043 jedel1043 requested a review from a team January 7, 2025 18:13
@hansl
Copy link
Contributor

hansl commented Jan 7, 2025

I suppose you mean maintaining a crate of job queues for ease of use?

Correct.

people still would want to know how to implement their own event loop

I would reply that they can always read the code and doc (assuming it's well written, of course).

@jedel1043
Copy link
Member Author

jedel1043 commented Jan 7, 2025

I would reply that they can always read the code and doc (assuming it's well written, of course).

I don't like assuming people will read the code base for questions they have, because most of the time they (unfortunately) don't 😅. Most users would open the examples folder, see that the "event_loop" example doesn't have an event loop, and either close the page or ask a question on how to do it.

examples/src/bin/futures.rs Outdated Show resolved Hide resolved
Copy link
Member

@nekevss nekevss 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 to me.

The examples look good. Was there any thought about putting together a blog post around the examples at some point to publish on the site. Could be a good post.

@jedel1043 jedel1043 marked this pull request as draft January 8, 2025 00:54
@jedel1043
Copy link
Member Author

Gonna put it on draft for a moment to add an example on how to use run_jobs_async, since there were some people asking how to execute the engine inside #[tokio::main]

@jedel1043 jedel1043 marked this pull request as ready for review January 8, 2025 04:32
@jedel1043
Copy link
Member Author

Should be good to review now.

@jedel1043 jedel1043 requested review from nekevss, hansl and a team January 8, 2025 04:33
@jedel1043 jedel1043 added this pull request to the merge queue Jan 9, 2025
Merged via the queue into main with commit a235976 Jan 9, 2025
13 checks passed
@jedel1043 jedel1043 deleted the better-event-loops branch January 9, 2025 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation update documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How do you properly call an async function under tokio?
3 participants