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

Upgrade pdfjs-dist to 2.4.456 #550

Closed

Conversation

kylemellander
Copy link
Contributor

#280

Due to issues caused with cross site loading of the pdf service worker, it is needed for us to upgrade the version of the pdfjs-dist package.

There was one deprecation that was raised in this upgrade that is addressed regarding the EventBus. The default global event bus was deprecated, so you have to manually manage it.

This updates to the latest version of pdfjs-dist.  The previous versions 
had a bug that caused cross site loading issues for some webpack builds.
With the newer version of pdfjs-dist, using the default EventBus has 
been deprectated. This sends in our own eventBus to manage the event 
messages.
@kylemellander
Copy link
Contributor Author

I'm still working on getting the build/test going, but wanted to put this out there to start the work/collaboration on this

The ES6 version requires support of ReadableStream causing issues.  
Rather than using a polyfill or mocking this, we should just assume that 
it has been compiled to a working syntax, which for the tests is ES5.
The updated pdfjs-dist library added an unscore to the location of the 
pageIndex.  This happens in real time, but since we are mocking it in 
the tests, we need to make sure to mock it correctly.
@kylemellander
Copy link
Contributor Author

I haven't tested all the different usages, but the ones I have tested, this works 👍.

This is done to match the need to manually set the eventBus on a 
LinkService
@wojtekmaj
Copy link
Owner

Hi,
thanks for this PR. Unfortunately, the newest stable PDF.js is 2.3.200 at the moment, so while your changes will certainly make it easier for me to do the upgrade, which I'm thankful for, I can't merge it right now.

@wojtekmaj wojtekmaj changed the title Upgrade pdfjs dist Upgrade pdfjs-dist to 2.4.456 Apr 15, 2020
@wojtekmaj
Copy link
Owner

Hi! 2.4.456 is marked as stable, so I'm gonna go ahead and upgrade pdfjs-dist.

Two questions:

  • There doesn't seem to be any warning, error or a failing test if no EventBus is provided. Is there something we're missing and should be tested either manually or automatically?
  • Why entry points for Jest had to be changed?

@wojtekmaj wojtekmaj self-requested a review June 2, 2020 11:07
@wojtekmaj wojtekmaj self-assigned this Jun 2, 2020
@wojtekmaj wojtekmaj added the dependencies Pull requests that update a dependency file label Jun 2, 2020
@kylemellander
Copy link
Contributor Author

@wojtekmaj Great news!

To answer your questions:

  • That seems like a good thing to add. I'll try to find some time to add that in.
  • I explained in the commit to make the change. It says:

The ES6 version requires support of ReadableStream causing issues.
Rather than using a polyfill or mocking this, we should just assume that
it has been compiled to a working syntax, which for the tests is ES5.

@wojtekmaj
Copy link
Owner

Thanks for your answers!

It seems like indeed the ES6 version of pdfjs-dist does not use ReadableStream anymore. It used to work fine. So, we can't use ES6 version by default just yet, because it's a breaking change. Not something you would expect when jumping from 2.3 to 24, but Mozilla can't really use semver, unfortunately.

Based on the insight you kindly shared, I've decided to raise another PR upgrading pdfjs-dist to 2.4.456, but keeping ES5 version for ALL entry files to avoid potential compatibility issues. I added you as a co-author there - hope you don't mind me crediting you this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants