-
Notifications
You must be signed in to change notification settings - Fork 294
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
Abstract async runtimes #1848
base: master
Are you sure you want to change the base?
Abstract async runtimes #1848
Conversation
Hi! Thanks for opening this pull request! 😄 |
b7352e9
to
080b6e8
Compare
@fzyzcjy can you check you are happy with approach then I need to fix some stuff about cargo.lock also how user can spawn or spawn_local ... |
@malivix Looked at the code very briefly and the rough approach seems OK to me! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1848 +/- ##
==========================================
- Coverage 99.27% 97.69% -1.58%
==========================================
Files 359 359
Lines 15127 15119 -8
==========================================
- Hits 15017 14771 -246
- Misses 110 348 +238 ☔ View full report in Codecov by Sentry. |
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.
Good job! Mainly some nits
@@ -362,6 +362,7 @@ | |||
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; | |||
CLANG_ENABLE_MODULES = YES; | |||
CURRENT_PROJECT_VERSION = "$(FLUTTER_BUILD_NUMBER)"; | |||
DEVELOPMENT_TEAM = FX7V3ATKWH; |
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.
nit: Is this your development team? Anyway just run ./frb_internal generate-run-frb-codegen-command-integrate --package frb_example/flutter_via_create
(and same command for frb_example/flutter_via_integrate) and it will be fixed
user-utils = ["dep:android_logger", "dep:oslog"] | ||
dart-opaque = ["dep:dart-sys-fork"] | ||
tokio-runtime = ["dep:tokio"] | ||
async-std-runtime = ["dep:async-std"] |
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.
nit: seems nuclei
is not there
tokio-runtime = ["dep:tokio"] | ||
async-std-runtime = ["dep:async-std"] |
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.
nit: shall we make the names more coherent. e.g. "rust-async, rust-async-tokio, rust-async-async-std, rust-async-nuclei" or something like that
@@ -22,6 +22,7 @@ impl BaseCodec for CstCodec { | |||
// frb-coverage:ignore-end | |||
} | |||
|
|||
#[allow(dead_code)] |
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.
nit: curious why is this dead code?
#[cfg(not(wasm))] | ||
pub use io::*; | ||
|
||
#[cfg(wasm)] | ||
mod web; | ||
#[cfg(wasm)] | ||
pub use web::*; |
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.
nit: is it really ok to remove these exports 👀
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.
nit: seems missing nuclei.rs
} | ||
|
||
#[derive(Debug, Clone, Copy, Default)] | ||
pub struct SimpleAsyncRuntime; | ||
|
||
impl BaseAsyncRuntime for SimpleAsyncRuntime { | ||
type JoinHandle<O> = wasm_bindgen_futures::JsFuture; | ||
|
||
#[cfg(feature = "rust-async")] |
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.
nit: seems no need for this cfg, since the whole web.rs is under that flag
/// Similar to tokio's `spawn_blocking`, except that you need to provide a second argumnet. | ||
/// If you are using flutter_rust_bridge, the second argument can be easily provided: | ||
/// Just use `FLUTTER_RUST_BRIDGE_HANDLER.thread_pool()`. | ||
/// | ||
/// More formally, the second argument is defined as: | ||
/// | ||
/// * When on web: The thread pool you want to use. | ||
/// * When on non-web: Unused, can be anything (since we use Tokio's built-in pool). |
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.
nit: these docs seems to be missing
frb_rust/src/rust_async/mod.rs
Outdated
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 great to add tests to ensure everything works.
The most thorough test will need to re-run frb_example/pure_dart
multiple times, each using one async runtime. However, that may be super slow.
(I will think about it a bit later)
frb_rust/src/rust_async/mod.rs
Outdated
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.
nit: maybe we can update doc a bit to tell future users about the new feature :)
- https://cjycode.com/flutter_rust_bridge/guides/cross-platform/async, changing things like "tokio" to "tokio / async-std / nuclei"
- Inside https://cjycode.com/flutter_rust_bridge/guides/miscellaneous folder, maybe add a file saying "customizing async runtime"
- and add a sentence in https://cjycode.com/flutter_rust_bridge/guides/concurrency/async-rust to point to that new doc
I am on travel will finalize it soon |
No worries, take your time and have a good vacation! |
@malivix - Is this going to get some love? 🥰, have been waiting for it for some time... Is there anything I can be of help with? |
Sorry I was in travel and a bit busy will try to finalize it this week |
Any update on this PR please? @malivix sorry to follow up. |
Changes
Close #1846 (EDITed by @fzyzcjy to use "Close" to make github auto trigger correlation)
Checklist
./frb_internal precommit --mode slow
(orfast
) is run (it internal runs code generator, does auto formatting, etc)../website
folder) are updated.Remark for PR creator
./frb_internal --help
shows utilities for development.