medooze / whip-whep-js

WHIP and WHEP clients javascript module
MIT License
65 stars 15 forks source link

Don't throw Error on fetch PATCH 501, 405 status #13

Closed cameronelliott closed 10 months ago

cameronelliott commented 11 months ago

From my reading of the whep draft, on a PATCH request to the resource/server a 501 or 405 status shouldn't abort or quit the current request.

Here is a small PR to fix that.

murillo128 commented 11 months ago

how is the error thrown on the patch request aborting the request? In the current code it is triggered inside a setTimeout call, so at most it can trigger an unhanded exception code. If called by the app, it should just catch the error and react appropriately.

How about just wrapping the calls to the trickle() method in a try/catch in the settimeots instead?

rgl commented 11 months ago

@murillo128 are you suggesting that the user should subclass WHEPClient and do the error handling? how to we known how to handle the error? maybe I'm missing something, but just ignoring the error feels like a strange error handling strategy, and should probably be handled by WHEPClient.

Something like the follow?

class PatchedWHEPClient extends WHEPClient {
    constructor() {
        super();
    }

    async trickle() {
        try {
            await super.trickle();
        } catch (error) {
            // ignoring the error feels odd.
            console.error('An error occurred in the trickle method:', error);
        }
    }
}
murillo128 commented 11 months ago

No, my point is that trickle() is a kind-of-private function and it is not needed to be called by the application in any case I am aware of

rgl commented 11 months ago

Indeed, it should stay private, but should not throw, it should recover by itself.

For ponderation, when running a react application (e.g. https://github.com/rgl/reolink-e1-zoom-playground/blob/main/react/whep.js) in development mode with npx webpack serve --mode=development --progress, when there is an un-handled promise exception, it kinda messes with the flow of things because it (correcly) shows an obtrusive error overlay:

image

cameronelliott commented 10 months ago

rgl said:

Indeed, it should stay private, but should not throw, it should recover by itself.

For ponderation, when running a react application (e.g. https://github.com/rgl/reolink-e1-zoom-playground/blob/main/react/whep.js) in development mode with npx webpack serve --mode=development --progress, when there is an un-handled promise exception, it kinda messes with the flow of things because it (correcly) shows an obtrusive error overlay:

image

I don't believe these exceptions turn into React errors like your image. I am using this lib in React, and in my app the PATCH request failures don't cause that to occur. I think thats because the exception occurs in/on the timeout context, so the exception just bubbles to the root without hitting react.

cameronelliott commented 10 months ago

Sergio said:

how is the error thrown on the patch request aborting the request? In the current code it is triggered inside a setTimeout call, so at most it can trigger an unhanded exception code. If called by the app, it should just catch the error and react appropriately.

How about just wrapping the calls to the trickle() method in a try/catch in the settimeots instead?

You are right, the patch error is not aborting the request, I was confused. Sorry.

You do get a couple errors in the chrome console (when using Chrome):

image

But honestly, after understanding this better, and understanding the exceptions are NOT causing the whep connection to fail, but just causing the console error logs, I think this is okay behavior at this time.

Thus, I will close this issue.