Fixing dependencies to support yarn2 strict mode#6942
Fixing dependencies to support yarn2 strict mode#6942zemd wants to merge 1 commit intoloopbackio:masterfrom zemd:chore/yarn2-pnp-strict-mode-support
Conversation
zemd
requested review from
agnes512,
bajtos,
emonddr,
jannyHou,
mschnee and
raymondfeng
as code owners
Signed-off-by: Dmitry Zelenetskiy <dmitry.zelenetskiy@gmail.com>
achrinza
left a comment
achrinza
left a comment
There was a problem hiding this comment.
Thanks for the PR! I've left some initial remarks 👇
| "@loopback/core": "^2.13.1" | ||
| }, | ||
| "dependencies": { | ||
| "@loopback/repository": "^3.3.0", |
There was a problem hiding this comment.
Would Yarn2 PnP be satisfied with it being referenced as a peerDependency instead?
The reason for peerDependency is so that only a single version of the package is used across the project and its dependencies (See #5959). Adding @loopback/repository as a hard dependency would break this paradigm.
Though it got me curious if there's any other LB4 packages that have the same issue.
We also probably should update our CI pipeline to test with Yarn2 PnP.
| "@loopback/core": "^2.13.1" | |
| }, | |
| "dependencies": { | |
| "@loopback/repository": "^3.3.0", | |
| "@loopback/core": "^2.13.1" | |
| "@loopback/repository": "^3.3.0" | |
| }, | |
| "dependencies": { |
There was a problem hiding this comment.
-1. Our extensions are intentionally using peer dependencies for @loopback/repository, as explained in the comment above.
To fix the problem you are observing with Yarn2 PnP, we should find the place that's depending on @loopback/openapi-v3 but not fulfilling it's peer dependency on @loopback/repository. I think we need to add @loopback/repository to peerDependencies of @loopback/rest.
There was a problem hiding this comment.
We also probably should update our CI pipeline to test with Yarn2 PnP.
That's a great idea 👍 Would you @achrinza mind opening a new GH issue for that?
| "access": "public" | ||
| }, | ||
| "dependencies": { | ||
| "@apidevtools/swagger-parser": "^10.0.2", |
There was a problem hiding this comment.
Any reason/benefit for the switch? AFAIK, both are actively maintained as aliases.
There was a problem hiding this comment.
Let's not mix multiple kinds of changes in a single pull request please.
Either switch the swagger parser implementation, or fix dependencies to enable yarn2, but don't do both.
mschnee
left a comment
mschnee
left a comment
There was a problem hiding this comment.
If adding support for yarn2/pnp, we should probably be diligent and also consider updating the travis configuration to support yarn2-based installations in addition to the current npm ci install.
| docs/_preview/ | ||
|
|
||
| # IDE files | ||
| /.idea |
There was a problem hiding this comment.
I believe we are intentionally not listing .idea files in .gitignore, advising users to setup a global ignore rule instead.
If you think it's time to change that, then please open a new pull request where we can discuss the change on it's own, independently on feature/bug fix work.
| "access": "public" | ||
| }, | ||
| "dependencies": { | ||
| "@apidevtools/swagger-parser": "^10.0.2", |
There was a problem hiding this comment.
Let's not mix multiple kinds of changes in a single pull request please.
Either switch the swagger parser implementation, or fix dependencies to enable yarn2, but don't do both.
| "@loopback/core": "^2.13.1" | ||
| }, | ||
| "dependencies": { | ||
| "@loopback/repository": "^3.3.0", |
There was a problem hiding this comment.
-1. Our extensions are intentionally using peer dependencies for @loopback/repository, as explained in the comment above.
To fix the problem you are observing with Yarn2 PnP, we should find the place that's depending on @loopback/openapi-v3 but not fulfilling it's peer dependency on @loopback/repository. I think we need to add @loopback/repository to peerDependencies of @loopback/rest.
| "@loopback/core": "^2.13.1" | ||
| }, | ||
| "dependencies": { | ||
| "@loopback/repository": "^3.3.0", |
There was a problem hiding this comment.
We also probably should update our CI pipeline to test with Yarn2 PnP.
That's a great idea 👍 Would you @achrinza mind opening a new GH issue for that?
|
This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity. |
|
This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one. |
4 participants
Yarn2 PnP strict mode requires that dependencies were strictly defined within packages which use them and yarn2 throw an exception during the project launch.
Checklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈