-
Notifications
You must be signed in to change notification settings - Fork 416
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
Remove optional peer dependencies #542
Remove optional peer dependencies #542
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code coverage decreased significantly.
I’ll take a look! |
I force pushed to remove the merge commit from master and rebased against the master so the branch is clean and up to date. I've also added more test to cover each scenarii: with & without optional peer deps. I've duplicated the whole peer dependencies tests because I didn't know how to achieve same test with a different |
Force pushed to make the PR up-to-date with the master branch. Also coverage dropped because there is an issue on Travis. The Node 6 build generate a good coverage while the Node 8 build generate less coverage: |
When fetching peer dependencies we now remove those which are optional. And optional dependencies can be defined using Yarn (only at the moment) when using the `peerDependenciesMeta` field.
Rebased again from master so it's tested on Node 10 & 12. |
@j0k3r Would you be interested in joining the Serverless Webpack team? |
@miguel-a-calles-mba Sure! |
What did you implement:
When fetching peer dependencies we now remove those which are optional.
And optional dependencies can be defined using Yarn (only at the moment) when using the
peerDependenciesMeta
field:I got that issue because jsdom recently added a peer dependency as optional: jsdom/jsdom#2605
The
peerDependenciesMeta
exist in Yarn since almost a year now: yarnpkg/yarn#6671How did you implement it:
If the peer dependency is defined as optional, I removed it from the peer dependencies.
How can we verify it:
Build a project which contains the
jsdom
deps as of15.2.0
.It'll try to fetch the
canvas
deps which is defined as peer dependency but optional.You should get that error when build the package using serverless:
With that PR the optional peer dependency will be ignored:
Todos:
Is this ready for review?: YES
Is it a breaking change?: NO