-
Notifications
You must be signed in to change notification settings - Fork 100
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(core): fast Wasm streaming in Rust #349
base: main
Are you sure you want to change the base?
Conversation
@@ -179,7 +186,6 @@ impl JsRealmInner { | |||
std::mem::take(&mut realm_state.js_build_custom_error_cb); | |||
std::mem::take(&mut realm_state.js_promise_reject_cb); | |||
std::mem::take(&mut realm_state.js_format_exception_cb); | |||
std::mem::take(&mut realm_state.js_wasm_streaming_cb); |
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 should probably still take this one just in case there are globals in the closure. Eventually I'd like to get rid of that explicit intermediate take
step but I can't guarantee right now we won't leak bits of the isolate right now.
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 agree with Matt here - can you verify this end-to-end in Deno repo if that doesn't cause problems before merging this 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.
LGTM. Net negative lines :)
Can this PR be landed now? |
Looks like this needs a re-merge but it doesn't look too bad. |
@littledivy Do you mind if I take a stab at re-merging and landing this? |
No description provided.