-
Notifications
You must be signed in to change notification settings - Fork 94
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(event streaming): configurable worker path, use SharedWorker #2080
Conversation
@@ -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"); |
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 might not be "worker.js".
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.
Was already fixing this. Done :)
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 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
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.
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)
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.
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 ?
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.
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
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.
If they requested multiple workers in the future we can do it in another PR.
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.
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.
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's still a single worker file for event streaming
I am aware of it, I meant event streaming + different tasks on workers
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.
I am aware of it, I meant event streaming + different tasks on workers
In this case they can change the config I guess.
@smk762 @gcharang after this is merged to dev, |
…rializing `EventStreamConfiguration`
@smk762 @gcharang I changed the worker type to be a shared worker, so the script in // use chrome://inspect in Chrome to debug the worker
// use about:debugging in Firefox to debug the worker
self.onconnect = function(event) {
var port = event.ports[0];
console.log('A new connection to the shared worker has been successfully established.');
port.onmessage = function(e) {
console.log(e.data);
};
}; Shared workers debugging can be accessed from |
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.
🔥
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.
according to this doc, and usage of shared worker..do you think we need it here just stick to dedicated worker without much stuffs going on in this 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.
LGTM 🔥
it was requested by @naezith since normal worker doesn't fit his needs in the flutter codebase. Plus, I think it's good to have event streaming worker shared and accessed from multiple contexts. |
* dev: feat(indexeddb): advanced cursor filtering impl (KomodoPlatform#2066) update dockerhub destination repository (KomodoPlatform#2082) feat(event streaming): configurable worker path, use SharedWorker (KomodoPlatform#2080) fix(hd_tests): fix test_hd_utxo_tx_history unit test (KomodoPlatform#2078) feat(network): improve efficiency of known peers handling (KomodoPlatform#2074) feat(nft): enable eth with non fungible tokens (KomodoPlatform#2049) feat(ETH transport & heartbeats): various enhancements/features (KomodoPlatform#2058)
This PR allows any worker path to be used instead of hardcoded
worker.js
file. The worker path can be specified usingworker_path
inevent_stream_configuration
, if not specifiedworker.js
will be used as default. Example: