FileLoader: Incorrect error handling. #22925
Conversation
|
@gkjohnson Does this PR look good to you? |
| if ( callbacks === undefined ) { | ||
|
|
||
| // When onLoad was called and url was deleted in `loading` | ||
| this.manager.itemError( url ); |
There was a problem hiding this comment.
I think we need to fire this.manager.itemEnd( url ); here as it does at the end of the "catch" block, too
There was a problem hiding this comment.
In the reported use case itemEnd() would be executed twice then. At this place and in the previous then() block.
There was a problem hiding this comment.
I don't think that's the case. If an error is thrown in the promise callbacks and the catch callback is run then the above itemEnd will not be hit. And either way the catch function already calls the itemEnd function at the bottom of it (line 218) so it should be called in this new branch too for consistency.
Another option would be to call itemEnd in a ".finally" callback, instead, so it always gets run
There was a problem hiding this comment.
Another option would be to call itemEnd in a ".finally" callback, instead, so it always gets run
That sounds like the cleanest solution to me.
There was a problem hiding this comment.
Great! I think this looks right to me, now!
leyiang
changed the title
|
Thanks! |
Related issue: Fixed #22923
Description
Once the fetch of FileLoader is fulfilled,
urlwill be deleted inloadingobject.If there are any error being throwed inside the
onLoadcallback,catchfunction of the fetch will throw confusing error message instead of real error.