ngx-extended-pdf-viewer icon indicating copy to clipboard operation
ngx-extended-pdf-viewer copied to clipboard

PDFScriptLoaderService fails to properly load the pdfjs viewer after destroyed.

Open Hypercubed opened this issue 1 year ago • 7 comments

Here is a tricky one.... I noticed that when running >v21, component tests started breaking. After quick a bit of investigation I'm pretty sure I've narrowed down the issue to the changes introduced in 21.0.0-alpha.0. Here is the breakdown:

21.0.0-alpha.0 introduces PDFScriptLoaderService which loads the pdfjs viewer inside the loadViewer method. The loadViewer method relies on the ngxViewerFileHasBeenLoaded event being triggered when loading the viewer mjs file. This works fine on initial load since the mjs file includes the event trigger. However, in unit/component testing the Angular app is destroyed and the PDFScriptLoaderService#destroy method is called to cleanup. The Angular app is then "rebooted" in the same window. But since the viewer is loaded as a module, it is not executed again (ES modules are loaded once and reused). The ngxViewerFileHasBeenLoaded event is never triggered and therefore the pdfjs is never initailzed.

Seams like we need a different way to trigger the listener inside loadViewer that doesn't rely on side effects inside the module.

I've only seen this in testing; since a app usually has the same lifecycle as the document/window.

Hypercubed avatar Aug 16 '24 20:08 Hypercubed

Tricky indeed. To be honest, I'm a clueless.

stephanrauh avatar Aug 16 '24 20:08 stephanrauh

Esiest fix I can up with is to cachebust the viewer mjs file. Two lines changed:

const viewerPath = this.getPdfJsPath('viewer') + '?v=' + new Date().getTime();

and in createScriptElement

script.type = sourcePath.includes('.mjs') ? 'module' : 'text/javascript';

Not the best solutions... since it also bypasses the browser cache.

Hypercubed avatar Aug 16 '24 20:08 Hypercubed

Guessing the better fix is to wrap these lines in a global function that gets called onload. https://github.com/stephanrauh/ngx-extended-pdf-viewer/blob/main/projects/ngx-extended-pdf-viewer/assets/viewer-4.5.713.mjs#L38804C1-L38812C31

Hypercubed avatar Aug 16 '24 20:08 Hypercubed

~Something like this (untested):~

DOESN'T WORK

  function ngxViewerFileHasBeenLoaded() {
    const event = new CustomEvent('ngxViewerFileHasBeenLoaded', {
      detail: {
        PDFViewerApplication: __webpack_exports__.PDFViewerApplication,
        PDFViewerApplicationConstants: __webpack_exports__.PDFViewerApplicationConstants,
        PDFViewerApplicationOptions: __webpack_exports__.PDFViewerApplicationOptions,
        webViewerLoad: __webpack_exports__.webViewerLoad,
      },
    });
    document.dispatchEvent(event);
  }

  if (document.readyState === "interactive" || document.readyState === "complete") {
    ngxViewerFileHasBeenLoaded();
  } else {
    document.addEventListener("DOMContentLoaded", ngxViewerFileHasBeenLoaded, true);
  }

Hypercubed avatar Aug 16 '24 20:08 Hypercubed

OK.. Here is the best I got:

In viewer.mjs:

  function ngxViewerFileHasBeenLoaded() {
    const event = new CustomEvent('ngxViewerFileHasBeenLoaded', {
      detail: {
        PDFViewerApplication: __webpack_exports__.PDFViewerApplication,
        PDFViewerApplicationConstants: __webpack_exports__.PDFViewerApplicationConstants,
        PDFViewerApplicationOptions: __webpack_exports__.PDFViewerApplicationOptions,
        webViewerLoad: __webpack_exports__.webViewerLoad,
      },
    });
    document.dispatchEvent(event);
  }

  window.ngxViewerFileHasBeenLoaded = ngxViewerFileHasBeenLoaded;

In pdf-script-loader.service.ts#loadViewer:

            document.addEventListener('ngxViewerFileHasBeenLoaded', listener);
            const script = this.createScriptElement(viewerPath);
            script.onload = () => {
                window.ngxViewerFileHasBeenLoaded();
            };
            document.getElementsByTagName('head')[0].appendChild(script);

Hypercubed avatar Aug 16 '24 20:08 Hypercubed

Could bypass the ngxViewerFileHasBeenLoaded altogether if ngxViewerFileHasBeenLoaded() returns the details directly.

I can do a PR if you are happy with having ngxViewerFileHasBeenLoaded as a global.

Hypercubed avatar Aug 16 '24 20:08 Hypercubed

I think we need a solution that does not pollute the global namespace. The best solution would be to wrap the viewer file in an Angular component. Unfortunately, I haven't managed to do that yet.

In the long run, I'd like to be able to have multiple independent instances of the viewer simultaneously. It seems many developers need this feature, and https://pdfviewer.net/extended-pdf-viewer/side-by-side uses a very clumsy work-around.

stephanrauh avatar Aug 17 '24 12:08 stephanrauh

@Hypercubed What's the current state of the art? Do you expect me to do something, or do you want to send a pull request?

Oh, and I wish you a Happy New Year!

stephanrauh avatar Dec 31 '24 17:12 stephanrauh

Happy new year to you!

I haven't look at this in a little while. I belive I had a not so great solution... I'll need to look again.

Hypercubed avatar Dec 31 '24 18:12 Hypercubed

@stephanrauh Update from me. Since August of last year we've made significant changes to our testing infrastructure (mainly swicthed from Cypress to Playwright for component testing). I belive because of that the issue has disappeared. As I mentioned it was only a testing issue with no obvious impact in production. I'm no longer blocked from updating to latest (v22).

That said I don't think the issue is gone. I can still see when running the pdf viewer in Storybook refreshing a the frame fails to restart the viewer correctly. Not sure if this will popup again in another way but it's no longer an issue for me.

Hypercubed avatar Mar 28 '25 21:03 Hypercubed

I'm tempted to close the issue. It doesn't feel right, but as you said, you've managed to find a workaround, and at the moment, I'm short of leisure time. I'm keeping the ticket open for a while because I don't want to accept I'm that short of available time. :)

BTW, I'm curious. I've discovered Playwright some time ago, and it works better for me than Cypress. But that's only my experience. I also know developers loving Cypress for a reason. What's your experience? Did the transition go smoothly? Do you regret or celebrate it?

stephanrauh avatar Mar 31 '25 19:03 stephanrauh

I've tried to write a Jest test for ngx-extended-pdf-viewer, but it seems pdf.js requires many resources and libraries that aren't included with standard JSDom. How did you solve the issue? I think this ticket requires the JavaScript code to execute, so mocking is not an option.

Do you have a smoke test you can give me?

stephanrauh avatar Apr 06 '25 10:04 stephanrauh

I think I've found the reason why your tests broke after updating. JavaScript modules (.mjs files) are treated different than traditional JavaScript files (.js). The former are executed once, no matter how often they are loaded, the latter are executed every time they are loaded.

stephanrauh avatar Apr 06 '25 12:04 stephanrauh

After trying various approaches, I believe cache busting it the best approach. I've implemented it for the viewer file in version 23.0.0. Set [forceFullReloadOfJavaScriptCode]="true". At the moment, the flag is false by default, but I suspect that's a silly default. When the PdfScriptLoaderService is triggered, it always should get a fresh copy of the viewer, independent of any other copy that may be in memory.

stephanrauh avatar Apr 06 '25 13:04 stephanrauh

BTW, I'm curious. I've discovered Playwright some time ago, and it works better for me than Cypress. But that's only my experience. I also know developers loving Cypress for a reason. What's your experience? Did the transition go smoothly? Do you regret or celebrate it?

I was a long time Cypress fan, but pretty much fully converted to Playwright. I'm very happy with the results. One thing that keep me on Cypress for a while was component testing but I found a way to combine Playwright with Storybook that I think is ideal for component testing.

My 2cents on Cypress vs Playwright:

  • Cypress is a business trying, and strugging FWICT, and it shows in the product. Playwright on the otherhand is a tool with no paywalled features.
  • Wrting tests in Playwright is more idiomatic Javascript and, from my experence, results in less flake. Cypress can be flake free as well but takes more effort.

As far as the conversion, it went very smooth. Not effertless but the tool cy2pw really helped.

Hypercubed avatar Apr 06 '25 16:04 Hypercubed

I've tried to write a Jest test for ngx-extended-pdf-viewer, but it seems pdf.js requires many resources and libraries that aren't included with standard JSDom. How did you solve the issue? I think this ticket requires the JavaScript code to execute, so mocking is not an option.

Do you have a smoke test you can give me?

Not using Jest nor JSDom. We're using Karma for unit tests, Playwright for integration and e2e tests, and Playwright + Storybook for component tests.

Hypercubed avatar Apr 06 '25 16:04 Hypercubed

I think I've found the reason why your tests broke after updating. JavaScript modules (.mjs files) are treated different than traditional JavaScript files (.js). The former are executed once, no matter how often they are loaded, the latter are executed every time they are loaded.

Yes... that's exactly it. I did test cache busting the .mjs file and that seamed to work. I don't want to dig out the old Cypress tests but think I am able to reproduce the issue in Storybook alone. I'll let you know when I can test 23.0.0.

Hypercubed avatar Apr 06 '25 16:04 Hypercubed

Version 23.0.0 has landed.

Enjoy! Stephan

stephanrauh avatar Apr 06 '25 17:04 stephanrauh