Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Conversation

Jordan-Hall
Copy link

@Jordan-Hall Jordan-Hall commented Jun 13, 2024

Fixes: #1093

Uploading image.png…

Signed-off-by: Jordan Hall <Jordan.Hall@mhr.co.uk>
@shamoon
Copy link
Contributor

shamoon commented Jun 13, 2024

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

@Jordan-Hall
Copy link
Author

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

@SimonFischer04
Copy link

Was just about to start working on the v4 port, but then saw this pr.
Tested this locally and everything works fine for me also.

Thanks for the great work @Jordan-Hall and @shamoon.

@Foxandxss
Copy link

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!

@DerekLiang
Copy link

Could someone please approve the PR and make a release? Thank you!

@andrey-voytenko
Copy link

please process a PR

@jlbales
Copy link

jlbales commented Jul 30, 2024

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.

@Bubiec
Copy link

Bubiec commented Aug 7, 2024

Is this library dead? Any good alternatives?

@Jordan-Hall
Copy link
Author

If its dark i'll release a fork or @shamoon might want too

@shamoon
Copy link
Contributor

shamoon commented Aug 7, 2024

I think the author just updates it infrequently but I dunno. I don’t plan to create a fork.

@VadimDez
Copy link
Owner

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!

@uright
Copy link

uright commented Aug 11, 2024

Thank you for @VadimDez for looking into this. Looking forward for the upgrade to take place. The #1109 dependency on pdfjs is preventing two of my applications to be released.

@Jordan-Hall
Copy link
Author

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

Copy link
Contributor

@shamoon shamoon left a 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

@shamoon
Copy link
Contributor

shamoon commented Aug 12, 2024

I think this should do it

--- 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.pdfViewer.setDocument(null as any);
+    }
+
     assign(PDFJS, 'disableTextLayer', !this._renderText);

     this.initPDFServices();

@csi-cdavis
Copy link

csi-cdavis commented Aug 13, 2024

The README should probably be updated to note that Angular 17+ is required for the latest PDFJS version due to the usage of Promise.withResolvers.

@shakoorattari
Copy link

When it will be released in npm?

@DerekLiang
Copy link

Can we publish a beta version of the NPM while we polishing the PR? Thank you!

@Bengejd
Copy link
Contributor

Bengejd commented Aug 20, 2024

@VadimDez any reason why this can't be merged?

@joh-klein
Copy link

joh-klein commented Aug 20, 2024

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?

Bengejd added a commit to Bengejd/ng2-pdf-viewer that referenced this pull request Aug 29, 2024
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.
@Bengejd
Copy link
Contributor

Bengejd commented Aug 29, 2024

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?

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.

@Jordan-Hall
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update pdfjs-dist to 4.2.67 or later