feat(pdfjs): upgrade to v4 - without breaking backwards compatibility #1128
Conversation
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.
|
@shamoon @joh-klein @DerekLiang @VadimDez Let me know if there is anything you would like me to change with the PR, but barring any new developments, I think this should satisfy the reasons the original PR was not merged in. |
|
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 project |
|
Very nice! Thank you so much for picking this up! |
| // @ts-expect-error This does not exist outside of polyfill which this is doing | ||
| if (typeof Promise.withResolvers === 'undefined') { | ||
| if (window) { | ||
| // @ts-expect-error This does not exist outside of polyfill which this is doing |
There was a problem hiding this comment.
Please combine the ifs and keep only one comment
There was a problem hiding this comment.
@VadimDez I consolidated the if statements ✅. The comments on the other hand are a different story... because we're checking to see if typeof Promise.withResolvers === 'undefined' in one line and then doing the actual assignment window.Promise.withResolvers = blah in the second line, Typescript will flag both lines as invalid due to Promise.withResolvers not being defined in the PromiseConstructor (because that is what we are Pollyfilling). And because it will throw an error, we need to suppress both statements one way or another.
So, if your problem with the duplicate statement is that it appears like we randomly duplicated the secondary @ts-expect-error I can certainly replace one (or both I suppose) with a simple @ts-ignore instead, but I think that leaving at least one @ts-expect-error comment in is for the best, as it helps document what the intention of that line of code is.
|
It looks great. Can we merge this? Thank you! |
|
Finally!!! Thank you all! 🥳 |
|
Thanks everyone who helped make this a reality! See you all next time a critical security flaw gets found 😉 |
This is a continuation of PR #1105 which seems by all accounts to have been abandoned by the original author (@Jordan-Hall). This PR implements all of the requested changes that @shamoon proposed, as well as the original PR implementation of upgrading pdfjs to v4.
Notable PR Differences:
Promise.withResolvers, a feature introduced relatively recently. The original PR feat(pdfjs): upgrade to v4 #1105, addressed this issue by upgrading Angular to version 17. However, I felt this approach was unnecessary, as the only blocker requiring the upgrade was the absence ofPromise.withResolvers. Instead, I chose to directly polyfill the function by referencing the official proposal polyfill and comments from other PDF-related repositories.If merged, this should close the following:
Pull Requests
Issues