-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
refactor: deno_runtime crate #8640
Conversation
d2ae184
to
ced32f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be really great to have a deno_runtime/examples
directory with a basic hello world example.
Added |
MainWorker::from_options(main_module.clone(), permissions, &options); | ||
worker.bootstrap(&options); | ||
worker.execute_module(&main_module).await?; | ||
worker.run_event_loop().await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: we should change the name of run_event_loop()
- it's neither accurate nor descriptive. The event loop is run in tokio. This method waits until all async ops have completed. wait_for_async_ops()
or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've already talked about it, ops are only one of a few things that are polled in run_event_loop
, besides ops there are pending module loads and pending dynamic imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait_for_various_async_things_to_complete()
then or wait_until_idle()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait_until_idle()
sounds ok, but let's do that in other PR
create_runtime_snapshot(&runtime_snapshot_path, js_files); | ||
} | ||
|
||
fn get_js_files(d: &str) -> Vec<PathBuf> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is only called once, "d" is always "rt".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's copy paste from cli/build.rs
, I suggest we do those minor cleanups in follow up PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - nice work.
This commit moves Deno JS runtime, ops, permissions and inspector implementation to new "deno_runtime" crate located in "runtime/" directory. Details in "runtime/README.md". Co-authored-by: Ryan Dahl <[email protected]>
This commit moves Deno JS runtime, ops, permissions and
inspector implementation to new "deno_runtime" crate.
Details in "runtime/README.md".
There is still some duplication of code, but overall diff is quite
small after all the recent refactors.
Closes #7928
Closes #2633