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

Replace pdfjs-dist build with legacy ES5 version #794

Merged
merged 5 commits into from
Sep 1, 2021

Conversation

njleonzhang
Copy link
Contributor

@njleonzhang njleonzhang commented Jun 4, 2021

react-pdf supports es5 itself, and its dependency pdfjs-dist supports es5 through dist/es5 folder. for entire supporting es5, react-pdf should require files in pdfjs-dist/dist/es5.
if we don't fix this issue, all users who depend on react-pdf have to analyze code of react-pdf, and find pdfjs-dist causes es5 incompatibility, then add pdfjs-dist babel transpiling config in their bundle tool(webpack) config. it don't make sense.

@wojtekmaj
Copy link
Owner

What is the issue we're trying to solve here? Does it crash on any of the supported browsers? Please raise an issue first so we can reproduce and discuss.

@wojtekmaj wojtekmaj changed the title fix es5 issue Replace pdfjs-dist build with es5 version Jun 4, 2021
@njleonzhang
Copy link
Contributor Author

njleonzhang commented Jun 6, 2021

What is the issue we're trying to solve here? Does it crash on any of the supported browsers? Please raise an issue first so we can reproduce and discuss.

Sorry, I have not found the browser list react-pdf supports. According to the document, I can know IE is not supported in v5.

// https://unpkg.com/[email protected]/dist/umd/entry.js
...
var pdfjs = _interopRequireWildcard(require("pdfjs-dist"));
...

react-pdf import pdfjs-dist in build code, this cause projects which use react-pdf import pdfjs-dist indirectly. react-pdf is es5 compatible, but pdfjs-dist is not by default.

This causes the projects which use react-pdf crash in browsers that do not support es6, and the users can be confused util they find the dependency relationship: user's project -> react-pdf -> pdfjs-dist.

Copy link

@0ex-d 0ex-d left a comment

Choose a reason for hiding this comment

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

@njleonzhang Have you been able to create an issue and link it to this PR?

@njleonzhang
Copy link
Contributor Author

@mrbaguvix why we need a issue, it's the process for pr merging?

pnpm-lock.yaml Outdated
@@ -0,0 +1,6336 @@
lockfileVersion: 5.3
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done @wojtekmaj

@yordis
Copy link

yordis commented Jul 15, 2021

Any idea when it could be release?

@njleonzhang
Copy link
Contributor Author

any plan to merge and release?

@wojtekmaj
Copy link
Owner

As of pdfjs-dist 2.9.359 es5 directory is gone. There is legacy directory though.

Not sure how to proceed, as we should really be using non-legacy code here. Perhaps a workaround based on webpack alias might be of any help.

@njleonzhang
Copy link
Contributor Author

ORZ!! The breaking change is released with a minor version?

@wojtekmaj
Copy link
Owner

Lol, that's classic PDF.js. Their versions do not follow semver at all. That's the very reason we have pdfjs-dist(and only pdfjs-dist) version locked.

@chinnick967
Copy link

I'm having this issue #808 in our project and wanted to see when this PR will be merged and released. Thanks!

Copy link
Owner

@wojtekmaj wojtekmaj left a comment

Choose a reason for hiding this comment

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

Need to rebase off newest main and change paths to from /es5 to /legacy

@andredantasrocha
Copy link

Hi guys, any idea when this is gonna be released? we are facing a similar issue in our project

@wojtekmaj
Copy link
Owner

It can't be merged before the fixes are made.

@grumpyolddev
Copy link

@njleonzhang any idea when you can get this fix done on the PR? Thank you!

Need to rebase off newest main and change paths to from /es5 to /legacy

@njleonzhang
Copy link
Contributor Author

njleonzhang commented Aug 27, 2021

I'v changed the path from /es5 to /legacy, but haven't tested yet.

@wojtekmaj wojtekmaj dismissed their stale review August 27, 2021 18:13

Updated PR

@andredantasrocha
Copy link

@njleonzhang Thanks for the changes!!! can't wait to use the new code after the PR is merged :)

@wojtekmaj wojtekmaj merged commit ca4453f into wojtekmaj:main Sep 1, 2021
@wojtekmaj wojtekmaj changed the title Replace pdfjs-dist build with es5 version Replace pdfjs-dist build with legacy ES5 version Sep 1, 2021
@wojtekmaj
Copy link
Owner

Thanks!

@andredantasrocha
Copy link

Amazing! Thanks guys!

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.

7 participants