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.

Sign up for GitHub

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

Jump to bottom

feat(event streaming): configurable worker path, use SharedWorker #2080

Merged
merged 12 commits into from
Mar 7, 2024
Merged

feat(event streaming): configurable worker path, use SharedWorker #2080

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e97d49c
Allow worker script URL to be specified in configuration
shamardy Mar 5, 2024
44052d4
fix wasm lint
shamardy Mar 5, 2024
41c8a83
make worker_path part of EventStreamConfiguration
shamardy Mar 5, 2024
f9ca486
fix expect message to use work_path
shamardy Mar 5, 2024
0682dd1
Refactor worker_path to use PathBuf
shamardy Mar 5, 2024
0476609
fix wasm lint
shamardy Mar 5, 2024
50e8f74
add doc comment for worker_path
shamardy Mar 5, 2024
4955bf4
Rename DEFAULT_WORKER_PATH to `event_streaming_worker.js`
shamardy Mar 5, 2024
d6120f3
Use SharedWorker instead of Worker in WASM event stream and initializ…
shamardy Mar 6, 2024
a161ada
use `default_worker_path` if `worker_path` was not provided when dese…
shamardy Mar 6, 2024
9bb1e8a
update browser compatibility link
shamardy Mar 6, 2024
5f015f6
add browser compatibility link for SharedWorker
shamardy Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions mm2src/mm2_event_stream/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub struct EventStreamConfiguration {
pub access_control_allow_origin: String,
#[serde(default)]
active_events: HashMap<String, EventConfig>,
pub worker_path: Option<String>,
onur-ozkan marked this conversation as resolved.
Show resolved Hide resolved
}

/// Represents the configuration for a specific event within the event stream.
Expand All @@ -51,6 +52,7 @@ impl Default for EventStreamConfiguration {
Self {
access_control_allow_origin: String::from("*"),
active_events: Default::default(),
worker_path: None,
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion mm2src/mm2_net/src/wasm_event_stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use mm2_core::mm_ctx::MmArc;
use serde_json::json;

const DEFAULT_WORKER_PATH: &str = "worker.js";

/// Handles broadcasted messages from `mm2_event_stream` continuously for WASM.
pub async fn handle_worker_stream(ctx: MmArc) {
let config = ctx
Expand All @@ -17,7 +19,8 @@ pub async fn handle_worker_stream(ctx: MmArc) {
"message": event.message(),
});

let worker = web_sys::Worker::new("worker.js").expect("Missing worker.js");
let worker_path = config.worker_path.as_deref().unwrap_or(DEFAULT_WORKER_PATH);
let worker = web_sys::Worker::new(worker_path).expect("Missing worker.js");
Copy link
Member

Choose a reason for hiding this comment

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

It might not be "worker.js".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was already fixing this. Done :)

Copy link
Member

@borngraced borngraced Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we might have another worker.js that serves a diff purpose In the future so why not make it balance_streaming_worker.js as default ?

Also, worker.js is a very basic worker that's not doing much and we might have a more complex worker in the future

Copy link
Collaborator Author

@shamardy shamardy Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I left it as worker.js for backward compatibility with this https://github.com/KomodoPlatform/react-komodefi-wasm/blob/streaming/worker.js ref #1978 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can make it balance_streaming_worker.js if agreed by @onur-ozkan and they would have to fix it in the react-komodefi-wasm branch

Also, worker.js is a very basic worker that's doing much and we might have a more complex worker in the future

Can you please elaborate on this @borngraced ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe we should get some feedback from the frontend devs. They may prefer to handle all the events from single worker file by filtering the incoming data.

It's still a single worker file for event streaming

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If they requested multiple workers in the future we can do it in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

the current worker script used for balance streaming is only logging the messages to the console..

It's just for demonstration, it doesn't have to be that way obviously.

You can handle all kinds of operations from a single worker file by filtering the incoming payload. As I mentioned, this preference should be defined by the frontend developers not by us as this affects their project structure directly.

Copy link
Member

Choose a reason for hiding this comment

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

It's still a single worker file for event streaming

I am aware of it, I meant event streaming + different tasks on workers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am aware of it, I meant event streaming + different tasks on workers

In this case they can change the config I guess.

let message_js = wasm_bindgen::JsValue::from_str(&data.to_string());

worker.post_message(&message_js)
Expand Down
Loading