Conversation
🦋 Changeset detectedLatest commit: df4c3f5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ale-adobe
left a comment
ale-adobe
left a comment
There was a problem hiding this comment.
Not a Rust expert, so won't comment on any language-specific things. Overall short-term workaround fix looks good to me. Have some non-blocking comments and suggestions. Thanks for the fix!
| { browser: 'chromium' }, | ||
| { browser: 'firefox' }, | ||
| { browser: 'webkit' } |
| { | ||
| "singleQuote": true | ||
| "singleQuote": true, | ||
| "trailingComma": "none" |
There was a problem hiding this comment.
I think the linter should do this already, but I suppose it doesn't hurt to also have it configured for Prettier.
packages/c2pa-web/src/lib/worker.ts
Outdated
| * | ||
| * I hope that one day this can be removed :) | ||
| */ | ||
| function wrapFunctions<T extends Record<string, (...args: any[]) => any>>( |
There was a problem hiding this comment.
nit: alternate naming suggestion
| function wrapFunctions<T extends Record<string, (...args: any[]) => any>>( | |
| function wrapFunctionsForErrorHandling<T extends Record<string, (...args: any[]) => any>>( |
packages/c2pa-web/src/lib/worker.ts
Outdated
| * prevents the proper handling of Error objects. As a workaround, we throw strings from our wasm-bindgen | ||
| * functions and convert them into errors here. | ||
| * | ||
| * I hope that one day this can be removed :) |
There was a problem hiding this comment.
nit: Prefer to leave commentary like this out of the actual function docs.
packages/c2pa-web/src/lib/worker.ts
Outdated
| /** | ||
| * Wraps all functions with additional error-handling code that converts any thrown strings into Error objects. | ||
| * This is only necessary because a bug (likely in wasm-bindgen, see https://github.com/wasm-bindgen/wasm-bindgen/issues/4961) | ||
| * prevents the proper handling of Error objects. As a workaround, we throw strings from our wasm-bindgen |
There was a problem hiding this comment.
| * prevents the proper handling of Error objects. As a workaround, we throw strings from our wasm-bindgen | |
| * prevents the proper handling of Error objects. As a workaround, we "throw" strings from our wasm-bindgen |
ale-adobe
left a comment
ale-adobe
left a comment
There was a problem hiding this comment.
Firefox skip LGTM. Thanks!
2 participants
Due to some mysterious behavior (likely in wasm-bindgen),
JsErrors were not being thrown properly by wasm-bindgen generated functions.As a workaround, we instead report our errors as
JsStrings, which we then "convert" into Error objects at the boundary between JS/WASM.I certainly do not love this as a fix. I am hopeful that the underlying issue in wasm-bindgen will eventually be fixed. See: wasm-bindgen/wasm-bindgen#4961
Additionally, this PR adds Firefox and Webkit to the test runner.