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: Support version specific pdf.worker.js url #971

Merged
merged 2 commits into from
Feb 21, 2023

Conversation

johnwest80
Copy link
Contributor

some environments can have multiple versions of the pdf viewer in the same browser instance, so url overrides need to have the ability to be specific to a version

some environments can have multiple versions of the pdf viewer in the same browser instance, so url overrides need to have the ability to be specific to a version
@johnwest80
Copy link
Contributor Author

@VadimDez hey there! so i have an app that runs micro front ends. there are some cases where they're built with different versions of your pdf viewer library. we need to do a pdf.worker.js url override, but since we have these different versions, each override needs to be specific to a version. this pr allows for that by setting a more specific window value that includes the version as part of the property.

i added tests, and am hoping we can get this merged asap? i know it's my first pr, and not sure how active you are with this, let me know if you have any questions about it at all!

@@ -6,7 +6,7 @@
"scripts": {
"ng": "ng",
"start": "ng serve",
"build": "ng build --configuration production=true",
"build": "ng build --configuration production",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this value of "production" should match what's in angular.json. not sure how this worked before this change with the latest version of angular. if i'm somehow out of sync on angular versions or something, feel free to discard this change.

@VadimDez
Copy link
Owner

VadimDez commented Feb 14, 2023

Do you run multiple pdfjs versions as well then? If so then it should pick the version automatically.
And what's the use case of having multiple different pdfjs versions? the library might not even work with some of the older versions of pdfjs

There's a way to provide path to your own pdf worker, you could pass whatever you want there. Could this be helpful for your case - https://github.com/VadimDez/ng2-pdf-viewer/blob/master/README.md#set-custom-path-to-the-worker ?

@johnwest80
Copy link
Contributor Author

we have apps that host components from different builds and platforms. we don't even know what versions other "shells" might be using, and we can't chance changing their behavior whatsoever. i know this is an edge case, but for us, it's a make or break edge case. i wrote this PR in such a way that it's totally non-breaking, so hopefully it won't be too much trouble to add this.

@johnwest80
Copy link
Contributor Author

and you mention there's a way to provide a path to your own pdf worker. that's what i'm using, but because of the possibility of version collisions, i couldn't just set the window property. i needed to scope it to a specific pdfjs version, which is what this pr is. that way, version a could still be pulling from a cdn, while version b could be pulling from a custom url. the pr ensures different versions don't step on each other's toes.

@VadimDez
Copy link
Owner

Can you please document it in the readme as well? Thanks

I got your idea, but would you use lets say: pdfjs v1 and pdf-worker v2 ?

@johnwest80
Copy link
Contributor Author

readme updated. hopefully that clarifies it. thx for working through this with me!

@johnwest80
Copy link
Contributor Author

@VadimDez do you think you can merge this?

@VadimDez VadimDez merged commit bfc4bb0 into VadimDez:master Feb 21, 2023
@johnwest80
Copy link
Contributor Author

thx for merging. can you publish a version to npm? appreciate it!

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.

3 participants