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

Drop webpack peerDependency from pdfjs-dist project #9733

Closed
hallysonh opened this issue May 17, 2018 · 7 comments
Closed

Drop webpack peerDependency from pdfjs-dist project #9733

hallysonh opened this issue May 17, 2018 · 7 comments
Labels

Comments

@hallysonh
Copy link

hallysonh commented May 17, 2018

What is the expected behavior?
The pdfjs-dist project should not include webpack as a peerDependency for it can really work without it when loading the scripts from build directory.

What went wrong?
I am using PDFJS in a component to visualize a PDF using Angular 6. No including webpack in my project (I do not need it at all), make a peerDependency warning popup every time I run any npm command or every time anyone install my component.

@nfantone
Copy link

nfantone commented Jun 6, 2018

Exactly. Having webpack as a "peerDependency" in a distributable package such as this is plain wrong and should be removed.

@timvandermeij
Copy link
Contributor

The peer dependency was added in #9249 because of worker-loader. Could you elaborate a bit on why it's wrong to have it in our repository? I'm fine with removing it if it's sure to not break things.

@nfantone
Copy link

nfantone commented Jun 6, 2018

Hi, @timvandermeij. Thanks for your reply!

webpack is a build tool and, as such, it usually only makes sense as a "devDependency" - or is otherwise restricted to the build pipeline of the library. Including it as a "peerDependency" means that users need to either install it locally (even though they may not need it/don't care for it) or ignore the warning that pops out every time on npm install.

I am not familiar with worker-loader, so take my comments with a pinch of salt. But if it is like any other webpack loader, they are typically only required as a build configuration step and nothing else. Also, it makes little sense in a context where the library is meant to be used outside the web (like Node).

Is pdf.js acting as a webpack plugin of sorts in some cases? If it is, it might make sense to include both the loader and webpack as a dependency. But, even then, I'd argue that that kind of support should be extracted to its own repo/module.

I took the liberty of forking the repo and taking everything webpack related out. It's still working absolutely fine for me in my Node application.

@mishawakerman
Copy link

Is there any plan to remove webpack as a dependency. @nfantone did you plan on submitting a PR?

@nfantone
Copy link

@mishawakerman I never really received an "official" response to this issue - and I still don't know why webpack is listed as a dependency. My (limited) intuition tells me this is part of a bigger issue where pdf.js is coupled to some part of its implementation that indirectly depends on webpack and needs to be refactored in some way.

@timvandermeij
Copy link
Contributor

This question is also asked on IRC and we got this answer: https://mozilla.logbot.info/pdfjs/20180606#c14862530-c14862541. In short, I'm not really familiar with Webpack bundling itself, but if it's really only for the example I guess we can remove it. Please check if that is the case before submitting a PR though.

@timvandermeij
Copy link
Contributor

Closing since doing this isn't really a good option given that #9248 will return if we do that. Instead, what we should do is #9580 which is discussed in IRC at https://mozilla.logbot.info/pdfjs/20180606#c14862530-c14862634.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants