Skip to content

Handle worker loading errors with a callback in newCrossOriginWorker#539

Merged
georgestagg merged 4 commits intor-wasm:mainfrom
Conjoint-ly:bugfix/invalid-baseurl-throw-error
May 23, 2025
Merged

Handle worker loading errors with a callback in newCrossOriginWorker#539
georgestagg merged 4 commits intor-wasm:mainfrom
Conjoint-ly:bugfix/invalid-baseurl-throw-error

Conversation

@Dual-Ice
Copy link
Contributor

Added an optional error callback to newCrossOriginWorker to handle worker loading issues such as network errors or creation failures. Updated related channels to log errors and reject promises when worker initialization fails. This improves error handling and resilience in cross-origin worker loading.

Added an optional error callback to newCrossOriginWorker to handle worker loading issues such as network errors or creation failures. Updated related channels to log errors and reject promises when worker initialization fails. This improves error handling and resilience in cross-origin worker loading.
@Dual-Ice
Copy link
Contributor Author

This is additional fix, because this one #461 solves only a half problem.

Copy link
Member

@georgestagg georgestagg left a comment

Choose a reason for hiding this comment

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

Thank you for debugging this issue further and providing this PR!

A minor nit is below, apologies for being overly picky about it.

`${config.baseUrl}webr-worker.js`,
(worker: Worker) => initWorker(worker),
(error: Error) => {
console.error("Worker loading error:", error);
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind if we remove the console.error() calls here and in the other channel implementations? If a user would like to write to the console they can do so by catching the WebRWorkerError rejection.

I don't mind the default console.error() emitted when onError is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that makes sense — thanks for pointing it out!
I'll remove the console.error() calls from the channel implementations and rely on consumers to handle logging via the rejected WebRWorkerError, as you suggested.
I'll leave the default console.error() when onError is undefined.
Pushing an update shortly.

@georgestagg
Copy link
Member

georgestagg commented May 21, 2025

Don't mind the CI failure, I don't believe they're from your changes. I'm currently working on upgrading the Fortran infrastructure in another PR and it can't find the compiler because it has been renamed in the latest main docker image.

@georgestagg
Copy link
Member

LGTM! Thanks!

@georgestagg georgestagg merged commit c6ee1e7 into r-wasm:main May 23, 2025
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants