Repository navigation
Reject asset load failures with an Error naming the URL - #149
Conversation
A failed background image or tasks-vision fileset reached callers as a bare DOM Event, with no message, so `e.message` was undefined and logs printed "Event". The image loader rejected with the <img> error event directly; it now rejects with an Error naming the path. MediaPipe rejects a failed wasm-loader <script> with that tag's error event, so init() converts it into an Error naming the script URL. Both keep the original event as `cause`. Adds e2e case B4, which points the fileset at a 404 and asserts the rejection is an Error naming it, and extends D6 to assert the logged image failure names the image. Both fail without this change. Fixes #142
🦋 Changeset detectedLatest commit: d3f489b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| throw new Error(`Failed to load the MediaPipe tasks-vision fileset from ${src}`, { | ||
| cause: err, | ||
| }); |
There was a problem hiding this comment.
suggestion: I forget if there's an explicit error handling pattern this repo has been using (if so, it probably makes sense to follow that), but I wonder if it makes sense to add in some custom error types to make these types of issues easier to catch?
There was a problem hiding this comment.
There isn't one. So I think it's better to just use plain errors since we have only 2 instances of this.
There was a problem hiding this comment.
Ok, yea, I mean I think this is ok, but I'm wondering if it makes sense to introduce something? We've got quite a few bare errors now and it might make sense to make them easier to catch. But this could be a follow up.
| img.onload = () => resolve(img); | ||
| img.onerror = (err) => reject(err); | ||
| img.onerror = (event) => | ||
| reject(new Error(`Failed to load background image from ${path}`, { cause: event })); |
There was a problem hiding this comment.
suggestion: Same thing here. Also, if there's a custom error type, then IMO it might be good to add that path value as a property?
| { | ||
| "compilerOptions": { | ||
| "lib": ["DOM", "ES2018"], | ||
| "lib": ["DOM", "ES2018", "ES2022.Error"], |
There was a problem hiding this comment.
nitpick: FYI that while this adds typing support for Error.prototype.cause, something like new Error('foo', { cause: new Error('bar') }) won't actually work on older browsers because cause is a newer feature.
To work around this in livekit-client, this was done instead: https://github.com/livekit/client-sdk-js/blob/db8d49c69950539c08167f0e505b0df2bd76f41a/src/room/errors.ts#L4-L19. This way cause is assiged directly on the error and not passed into the constructor via super. If you migrate to custom errors instead of Error, I'd recommend you do something similar to handle that cause property.
Browsers without support for the Error constructor's cause option drop it silently, so the original Event was lost there. cause is now assigned on the error after construction, and the ES2022.Error lib entry is no longer needed.
B4 now checks that the fileset rejection carries the script tag's error Event as cause. It fails if errorWithCause stops setting cause.
Fixes #142.
What
A failed background image or tasks-vision fileset reached callers as a bare DOM
Event.e.messagewasundefined,String(e)was"[object Event]", and the library's own log printedEvent. Two places produced it:loadAndSetBackgrounddidimg.onerror = (err) => reject(err), which rejected with the<img>error event. It now rejects withError("Failed to load background image from <path>").ImageSegmenter.createFromOptionsloads its loader through a<script>tag and rejects with that tag's error event. That code is in@mediapipe/tasks-vision, soinit()catches the rejection. If it's anEvent,init()rethrowsError("Failed to load the MediaPipe tasks-vision fileset from <script src>"). Any other failure passes through unchanged. MediaPipe's model fetch already throws anErrorthat includes the path, so it isn't wrapped.Both errors keep the original event as
cause. It's assigned on the error after construction, not passed as the constructor'scauseoption, because browsers that don't support that option drop it silently.Test
Errorwhose message contains the missing path.missing-bg.png.Both fail with the source change reverted. The full e2e suite passes (20/20).
Includes a patch changeset.