[api-minor] Remove the Webpack-only npm dependencies from pdfjs-dist (PR 11418 follow-up)
#11474
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…` (PR 11418 follow-up) Currently *all* users of `pdfjs-dist` are forced to install the `webpack` and `worker-loader` packages, despite the fact that they are *only* relevant if the `webpack.js` file is being used (with a custom Webpack build). This really doesn't seem great, especially since those packages are the only remaining dependencies in the `pdfjs-dist` library, and it thus seem more reasonable overall that Webpack users handle those dependencies themselves. To prevent unnecessarily cryptic runtime failures, when people update to newer `pdfjs-dist` versions, the `webpack.js` file was updated to explicitly check for the existence of the `worker-loader` package and error otherwise. Furthermore, note that `webpack` was only listed as a dependency because of the `worker-loader` package itself (see issue 9248). Obviously these changes may not be seen as great by Webpack users who rely on `pdfjs-dist`, since it forces them to handle the dependencies themselves, however it should improve things considerably for "general" users of `pdfjs-dist` by not burdening them with unnecessary dependencies. These sort of changes are also in line with other recent changes, see PR 11418, which removed built-in fake worker loader code for specific JS builders/bundlers/frameworks. This work was prompted not only by a desire to simplify/clean-up old code, but also to lessen future support burden since the PDF.js contributors cannot be assumed to be experts in various JS bundlers.
Snuffleupagus
force-pushed
the
pdfjs-dist-rm-dependencies
branch
from
51594a0
to
a0cf67d
Compare
timvandermeij
approved these changes
Jan 6, 2020
|
Given the number of likes on that issue and given the fact that we want to reduce the amount of maintenance for the various bundler solutions, this seems like the easiest solution to me as well and should benefit most users. Thanks! |
|
Looking at open issue related to Webpack, it seems that they're either "fixed" by this PR or basically support requests related to Webpack bundling/usage (given that no current PDF.js contributors seem to have much Webpack knowledge/experience it seems unlikely that they'll ever be "officially" answered). All in all, is it actually meaningful to keep those issues open? |
|
Not really. I'll go over them and close them if they are not actionable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently all users of
pdfjs-distare forced to install thewebpackandworker-loaderpackages, despite the fact that they are only relevant if thewebpack.jsfile is being used (with a custom Webpack build).This really doesn't seem great, especially since those packages are the only remaining dependencies in the
pdfjs-distlibrary, and it thus seem more reasonable overall that Webpack users handle those dependencies themselves.To prevent unnecessarily cryptic runtime failures, when people update to newer
pdfjs-distversions, thewebpack.jsfile was updated to explicitly check for the existence of theworker-loaderpackage and error otherwise.Furthermore, note that
webpackwas only listed as a dependency because of theworker-loaderpackage itself (see issue 9248).Obviously these changes may not be seen as great by Webpack users who rely on
pdfjs-dist, since it forces them to handle the dependencies themselves, however it should improve things considerably for "general" users ofpdfjs-distby not burdening them with unnecessary dependencies.These sort of changes are also in line with other recent changes, see PR #11418, which removed built-in fake worker loader code for specific JS builders/bundlers/frameworks. This work was prompted not only by a desire to simplify/clean-up old code, but also to lessen future support burden since the PDF.js contributors cannot be assumed to be experts in various JS bundlers.
Given that I cannot image any PDF.js contributor being keen on maintaining/supporting a separate library and npm package, similar to
pdfjs-dist, with proper built-in Webpack support; I'm thus suggesting that this PR is probably the easiest way to fix #9580.