search

ApryseSDK / webviewer-ui

WebViewer UI built in React
Other
416 stars 353 forks source link

onError callback does not get called for all errors #269

Closed butsjoh closed 5 years ago

butsjoh commented 5 years ago

Hi,

If you look at https://github.com/PDFTron/webviewer-ui/blob/1be39dcde7af4058ceb704a36a1eb2db694bdfd6/src/helpers/loadDocument.js#L174 it will only allow events of type 'string' or 'InvalidPDF' to be dispatched to the global window handler (loaderror).

Now we have a document that seems to be corrupted (an office document with extension xlsx). An indeed the webviewer is not capable of loading it but it seems in this case the global handler is never called because the error type is "error" and not one of the ones mentioned above. See attached screenshot for the error we get.

I suggest not the filter on any type of error and just let it be handled by the global handler. That way we can setup some logging for ourselves to deal with this kind of error.

image

justinjung04 commented 5 years ago

Hello,

Thank you for bringing this up. It is true that the existing error handling is not flexible to allow custom error handling, and we definitely need to improve it. We will put this item in a high priority after we release 5.1, and give you an update in this thread.

ZhijieZhang commented 5 years ago

Hi, the issue you are reporting is resolved here: https://github.com/PDFTron/webviewer-ui/pull/336/files#diff-520cee8bacc6954e6a18e3477a9baf46R200

This change will be included in the next patch release(5.2.3), which should be released this or the next week.

I will update this thread again after 5.2.3 is released. Thanks for bringing this to our attention!

henry-chris commented 5 years ago

@ZhijieZhang I've been watching this a little. Is the errorModal now hooked up fully to the raw promise for document loading? Meaning any error triggers the modal?

If true, does this also happen when hooked up to the Webviewer server and loading through the websocket connection?

ZhijieZhang commented 5 years ago

@henry-chris The errorModal still remains the same behavior after the above PR is merged. This means that it won't be opened by errors which didn't open it before.

However, the loaderror event will be fired even if the error is an object now. This means you could listen to the loaderror in your App and use instance.showErrorMessage to open the errorModal and customize the error message.

If true, does this also happen when hooked up to the Webviewer server and loading through the websocket connection?

I'm not familiar with the WebViewer Server so I can't guarantee what I'm saying next is true. I took a brief look at the implementation of the BlackBoxPartRetriever and BlackBoxDocument, which are the two classes used by WebViewer Server on the client side. It seems that the above PR should help you be able to catch more errors by listening to the loaderror event, but not all of them. I would suggest you to try out the latest WebViewer(5.2.3) and see how it goes.

In my opinion, error handling in WebViewer still has a room for improvement. Now errors could be thrown by the PartRetriever or Document during the loading process and some of them might be swallowed.

I think I will close this issue for now as it should be addressed in the latest WebViewer. Please feel free to reopen the issue and have further discussions here. If you'd want us to take a closer look at the connection between errorModal and WebViewer Server, I would recommend you to submit a ticket. As always, we'd happy to help you!

ZhijieZhang commented 5 years ago

@butsjoh This issue should be addressed in the WebViewer 5.2.3. https://github.com/PDFTron/webviewer-ui/blob/ec7c989005f2ccc1f2a1c4b1b79626c4cdbafac4/src/helpers/loadDocument.js#L199-L201

You can download the latest package here: https://www.pdftron.com/downloads/WebViewer.zip I will close the issue for now and please feel free to reopen it if you found any related issues. Thanks for your patience!