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

[breaking] Make file-loader a peerDependency #970

Merged
merged 3 commits into from
Jul 27, 2022
Merged

[breaking] Make file-loader a peerDependency #970

merged 3 commits into from
Jul 27, 2022

Conversation

rpaasche
Copy link
Contributor

Define devDependency as devDependencies.

Otherwise users of this package hast to use the same webpack version. Currently ther are some copatibility issues with create-rect-app projects and the file-loader version in this project.

@wojtekmaj
Copy link
Owner

See #911 (comment)

package.json Outdated
"make-cancellable-promise": "^1.0.0",
"make-event-props": "^1.1.0",
"merge-class-names": "^1.1.1",
"merge-refs": "^1.0.0",
"pdfjs-dist": "2.12.313",
"prop-types": "^15.6.2",
Copy link
Owner

Choose a reason for hiding this comment

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

This one is a normal dependency though. It gets imported in virtually all components. If it's missing, the project won't build.

Copy link
Contributor Author

@rpaasche rpaasche Mar 21, 2022

Choose a reason for hiding this comment

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

Right but it's a compiletime dependency (devDependency) the final build includes no information about prop-types.

Copy link
Owner

@wojtekmaj wojtekmaj Mar 21, 2022

Choose a reason for hiding this comment

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

That depends. The code built on my side does include propTypes, so if you use it in your project, prop-types package will be required.

It's up to developer's bundler configuration wherever propTypes are being removed or not.

Even if we decide to strip it off on our side, which doesn't make a lot of sense to me, then we would need to ship development and production versions of the code, and still, development version would require prop-types.

So, dependency must stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it back.

@rpaasche
Copy link
Contributor Author

Marked file-loader as peer now.

@wojtekmaj wojtekmaj changed the title fix: #911 devDependecies should be marked as such [breaking] Make file-loader a peerDependency Mar 23, 2022
@wojtekmaj wojtekmaj changed the title [breaking] Make file-loader a peerDependency [breaking] Make file-loader a peerDependency Mar 23, 2022
@wojtekmaj
Copy link
Owner

This looks good to me. We will have to wait for the next breaking version though, as this requires user to install file-loader manually to use Webpack-specific entry.

@pleerock
Copy link

yeap so there is a major problem for the users using react-pdf with other bundlers like vite and others:

└─┬ react-pdf
  └─┬ file-loader
    └── ✕ missing peer webpack@"^4.0.0 || ^5.0.0"

It requires webpack installation, but I do not use or need webpack. I don't feel this PR will resolve this issue, because I'll have to install file-loader (thus webpack) anyway.

@rpaasche
Copy link
Contributor Author

The second commit of this PR marks the peer as optional, so neither NPM nor yarn will install it without you to add it to your own package.json.

@wojtekmaj wojtekmaj merged commit a6f77c9 into wojtekmaj:main Jul 27, 2022
wojtekmaj added a commit that referenced this pull request Jul 27, 2022
Co-authored-by: Wojciech Maj <kontakt@wojtekmaj.pl>
wojtekmaj added a commit that referenced this pull request Jul 27, 2022
Co-Authored-By: Wojciech Maj <kontakt@wojtekmaj.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants