Upgrade pdfjs-dist to 6.0.227 for latest stable PDF.js support. - #2113
Upgrade pdfjs-dist to 6.0.227 for latest stable PDF.js support.#2113xbonell-sage wants to merge 3 commits into
Conversation
Adapt LinkService, AnnotationLayer, and tests for PDF.js 6 API changes, and configure the Vitest worker so browser tests run reliably.
wojtekmaj
left a comment
wojtekmaj
left a comment
There was a problem hiding this comment.
In general, not bad; a couple of notes that I'd be happy to work on further. However the breaking change in PDF.js itself should be studied, and especially the lifted minimum browser version requirements may be particularly problematic and may require a major version on out side as well.
| div: layer, | ||
| imageResourcesPath, | ||
| linkService, | ||
| linkService: linkService as unknown as PDFLinkService, |
There was a problem hiding this comment.
Not a fan of type assertions, let's fix that in a way that does not involve lying to TypeScript
|
|
||
| try { | ||
| new pdfjs.AnnotationLayer(annotationLayerParameters).render(renderParameters); | ||
| let cancelled = false; |
There was a problem hiding this comment.
makeCancellable should handle that by default without the need of manually checking if promise has been cancelled.
| const cancellable = makeCancellable( | ||
| new pdfjs.AnnotationLayer({ | ||
| ...annotationLayerParameters, | ||
| linkService: linkService as unknown as PDFLinkService, |
There was a problem hiding this comment.
Again, I would rather not lie to TypeScript here.
| executeSetOCGState(): Promise<void> { | ||
| return Promise.resolve(); | ||
| } |
There was a problem hiding this comment.
Or:
| executeSetOCGState(): Promise<void> { | |
| return Promise.resolve(); | |
| } | |
| async executeSetOCGState(): Promise<void> { | |
| // Intentionally empty | |
| } |
There was a problem hiding this comment.
I don't understand these changes yet...
There was a problem hiding this comment.
PDF.js 6 can render more [role="presentation"] nodes than textContent.items.length when marked content is included. Added a getRenderedTextItemCount() helper with a comment explaining this, instead of comparing against desiredTextItems.length.
There was a problem hiding this comment.
I don't understand these changes yet...
There was a problem hiding this comment.
Switched from asserting PDF.js internal _page* fields to public callback fields (pageNumber), which is what onLoadSuccess actually exposes. Updated the PR with a comment in the spec explaining this.
|
|
||
| # Project-generated directories and files | ||
| __screenshots__ | ||
| .vitest-attachments |
There was a problem hiding this comment.
Relevant? Why was this needed now?
Extend SimpleLinkService for proper typing, simplify AnnotationLayer cancellation, clarify test assertions, and remove unnecessary gitignore entry.
|
Thanks for the review! I've updated the PR addressing the inline comments:
About the browser requirements / major version: noted that PDF.js 6 raises minimums (Chrome 125+, Safari 18+). I agree that probably this should ship as a react-pdf major separately. Context / motivation: We needed to upgrade pdfjs-dist in react-pdf because we also use pdfjs-dist directly in our application. Keeping both on the same major version avoids subtle runtime and type mismatches (worker URL, API surface, and shared document APIs). |
858ed6c to
b00fb0b
Compare
2 participants
Adapt LinkService, AnnotationLayer, and tests for PDF.js 6 API changes, and configure the Vitest worker so browser tests run reliably.