Clean cancel #843
Clean cancel #843
Conversation
Don't call pdf.getPage() if Document is destroyed
|
Woo, lots of goodness in this PR! I'm especially curious about the Please allow more time for me to review - extremely busy in personal life recently. |
src/Page.jsx
Outdated
| // eslint-disable-next-line no-underscore-dangle | ||
| if (pdf && pdf._transport && pdf._transport.destroyed) { | ||
| this.setState({ page: false }); | ||
| this.onLoadError('Attempted to load a page, but the document was destroyed'); |
There was a problem hiding this comment.
onLoadError expects Error.
| this.onLoadError('Attempted to load a page, but the document was destroyed'); | |
| this.onLoadError(new Error('Attempted to load a page, but the document was destroyed')); |
src/Document.jsx
Outdated
| // If pdfjs loading is in progress, let's destroy it | ||
| if (this.loadingTask) this.loadingTask.destroy(); | ||
|
|
||
| // If FileReader loading is in progress, let's abort it | ||
| if (this.loadFromFileTask) { | ||
| this.loadFromFileTask.abort(); | ||
| } |
There was a problem hiding this comment.
Not a fan of this formatting inconsistency, but honestly, that should have been linter's job.
src/shared/utils.js
Outdated
| return { | ||
| promise, | ||
| abort: () => { | ||
| reader.abort(); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
I'm wondering, if perhaps making the API consistent with make-cancelable-promise by changing abort to cancel wouldn't be a bettter choice.
There was a problem hiding this comment.
Both ok for me. I am thinking: 'cancel' is something like "I did the request, but I don't want it anymore, don't send what ever you have, no matter if finished or not" (which is what cancelable-promise does). "Abort" on the other hand is "Stop what you are doing right now". But from our perspective I guess what we want indeed is the same in both cases: don't call any callbacks. So 'cancel' would make sense.
- call onLoadError() with Error() - inline if() for code consistency
wojtekmaj
force-pushed
the
main
branch
14 times, most recently
from
5e59480
to
41d6126
Compare
wojtekmaj
force-pushed
the
main
branch
3 times, most recently
from
ccad668
to
6bb29a9
Compare
wojtekmaj
force-pushed
the
main
branch
4 times, most recently
from
653aa2c
to
9bb8a85
Compare
wojtekmaj
force-pushed
the
main
branch
7 times, most recently
from
2d488c1
to
a077571
Compare
wojtekmaj
force-pushed
the
main
branch
5 times, most recently
from
1f758cc
to
ce6db03
Compare
wojtekmaj
force-pushed
the
main
branch
2 times, most recently
from
42a8c3c
to
0d68fee
Compare
wojtekmaj
force-pushed
the
main
branch
4 times, most recently
from
d0aabb2
to
736f3f3
Compare
I have a suggestion about cleaner error handling, if you like.
(Note that this is my first pull request ever, sorry if I do something wrong)
I know that react-pdf is not meant to be used like this anyway, but it seems it would be an easy fix.
(commit "cancel Page load if Document destroyed")
(commit "abort file load on document reload/unmount")
(commit "add test for Page outside Document")
Testing:
1. "cancel Page load if Document destroyed"
BEFORE:
TypeError: "this.messageHandler is null"
NOW:
callback .onLoadError("Attempted to load a page, but the document was destroyed")
2. "abort file load on document reload/unmount"
BEFORE:
"Warning: Can't perform a React state update on an unmounted component"
NOW:
no error in console (file load was aborted, setState wasn't called)