-
-
Notifications
You must be signed in to change notification settings - Fork 421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(pdfjs): upgrade to v4 #1105
Conversation
Signed-off-by: Jordan Hall <[email protected]>
Oh nice, I had thought that v4 would require substantive changes to the codebase but it seems the v3 changes were enough. Testing this locally things seem to work fine for me—great work. Ps. this also closes #1078 |
Yea it was thank you so much. Want to use this but hard time passing it through security :( even with your partial fix |
Was just about to start working on the v4 port, but then saw this pr. Thanks for the great work @Jordan-Hall and @shamoon. |
Any updates on this? We have complains about this vulnerability and I am trying to hold it off but we might need to either fork or replace this library. Thanks for the work tho! |
Could someone please approve the PR and make a release? Thank you! |
please process a PR |
Do we know how much longer until this is released? We may have to go with another library if it is not resolved this week. |
Is this library dead? Any good alternatives? |
If its dark i'll release a fork or @shamoon might want too |
I think the author just updates it infrequently but I dunno. I don’t plan to create a fork. |
I see few issues with this PR. One issue that I can reproduce is whenever I can toggle "Show all pages" - the same pdf will be appended on top of already rendered pdf. @shamoon thank you so much for taking a look and trying to manage PRs/Issues! |
Hmm ok i'll take a look at that sorry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to fix the show all pages issue for me (consolidating the import thing isnt important obv but it looks nice to me =):
--- a/src/app/pdf-viewer/pdf-viewer.component.ts
+++ b/src/app/pdf-viewer/pdf-viewer.component.ts
@@ -32,7 +32,6 @@ import type {
PDFViewerOptions,
ZoomScale
} from './typings';
-import { PDFSinglePageViewer } from 'pdfjs-dist/web/pdf_viewer.mjs';
import { GlobalWorkerOptions, VerbosityLevel, getDocument } from 'pdfjs-dist';
if (!isSSR()) {
@@ -64,7 +63,7 @@ export class PdfViewerComponent
public eventBus!: PDFJSViewer.EventBus;
public pdfLinkService!: PDFJSViewer.PDFLinkService;
public pdfFindController!: PDFJSViewer.PDFFindController;
- public pdfViewer!: PDFJSViewer.PDFViewer | PDFSinglePageViewer;
+ public pdfViewer!: PDFJSViewer.PDFViewer | PDFJSViewer.PDFSinglePageViewer;
private isVisible = false;
@@ -435,6 +434,10 @@ export class PdfViewerComponent
}
private setupViewer() {
+ if (this.pdfViewer) {
+ this.resetPdfDocument();
+ }
+
assign(PDFJS, 'disableTextLayer', !this._renderText);
this.initPDFServices();
Actually I may have spoken to soon, it does fix rendering 1 page but I still see a duplicate when toggling. I can play with it again another time (unless you sort it in the meantime of course =)
Edit: see below
I think this should do it
|
The README should probably be updated to note that Angular 17+ is required for the latest PDFJS version due to the usage of |
When it will be released in npm? |
Can we publish a beta version of the NPM while we polishing the PR? Thank you! |
@VadimDez any reason why this can't be merged? |
Well – since the issues noted by @VadimDez aren't fixed (even though @shamoon already wrote how to fix them), this will not move forward. @Jordan-Hall can you please update the PR with the changes so this even has the chance of being looked at and merged? |
This is a continuation of PR VadimDez#1105 which seems by all accounts to have been abandoned by the original author. However, the notable difference is that instead of pushing out a breaking change & forcing an upgrade to Angular 17 simply for Promise.withResolvers, instead I opted to Pollyfill it directly, as that is the only reason the upgrade was necessary (from what I could tell). I implemented the suggestions that @shamoon suggested in the original PR comments as well.
Since @Jordan-Hall hasn't responded in a while, I decided to resurrect the PR myself & address the issues brought forth. #1128 should satisfy those issues. |
It was on my list today. Sorry I no longer work for the company that needed this but was doing it today. But thank you so much for sorting this. I know I need it soon for personal projec |
Fixes: #1093