-
Notifications
You must be signed in to change notification settings - Fork 105
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(viewer): Upgrade pdf.js to latest for modern browsers #1441
Conversation
984b956
to
722ef51
Compare
|
||
cy.visit('/', { | ||
onBeforeLoad(win) { | ||
// Workaround for fetch detection in cypress mocking. https://github.com/cypress-io/cypress/issues/95 | ||
delete win.fetch; // eslint-disable-line no-param-reassign | ||
cy.stub(win, 'fetch').resolves({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirmed this change still triggers the expected error both on this branch and on master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good approach to help us move forward in the near term and pull in fixes for PDFjs while hanging onto IE support. Do we need to consider any type of feature flip type option to turn this on and off?
I wouldn't bother with a feature flip, since it's essentially the same process to roll back to a prior version of the SDK. |
743c108
to
ddc8ca5
Compare
@karelee7, seeing lots of new end-to-end test failures. Does Find work? Presentations? |
@jstoffan Ahh lemme see...so for find, it looks like it's having trouble and for presentations, they are either really slow or the file preview doesn't load :( Update! I figured it out with help from Conrad, will update commit now! |
af85bb1
to
b9b1e4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing the following error for all PDFs with this patchset:
Cannot read properties of undefined (reading 'EventBus')
Talked with Conrad, we may need to use google compiler + the legacy build of pdfjs for now, and then circle back to babel + the non-legacy build. But I'm going to dig a bit more tonight and tomorrow to see if I can get it to work, if not, then we'll go the google compiler + legacy route for now |
@karelee7, we're making progress, but I found a couple issues while spot-checking:
|
@jstoffan The fix for find next/prev buttons is in already and as you saw, the tooltip fix was here: box/box-annotations#696 and has been merged in :) |
@ConradJChan will need to review, as I can't approve a PR I opened. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, great job!
Huge thanks to @karelee7 for testing and validating these changes: