Add peer deps for postcss and e2e tests #39080
Merged
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
noahtallen
commented
Feb 24, 2022
packages/e2e-tests/package.json
Outdated
| @@ -37,6 +37,11 @@ | |||
| "puppeteer-testing-library": "^0.5.0", | |||
| "uuid": "^8.3.0" | |||
| }, | |||
| "optionalDependencies": { | |||
There was a problem hiding this comment.
Should these be peer or optional? I wasn't sure what to do, as I know puppeteer could be a very big thing to have to bring in. (At the same time, for the e2e-tests package, it kind of makes sense to need it.)
There was a problem hiding this comment.
Actually, I just moved these to peers. Let me know what you think
There was a problem hiding this comment.
We have been doing all we can to avoid having puppeteer in the node_modules because it downloads Chromium as part of the post-install process. puppeteer-core is more difficult to configure and integrate, but it lets us defer that painful moment until someone runs e2e tests for the first time:
gutenberg/packages/scripts/scripts/test-e2e.js
Lines 31 to 33 in ebb4a8a
In the past, we would alias puppeteer with puppeteer-core but it wasn't reliable with npm 6 so changed the strategy. Do you know which package declares puppeteer as a peer dependency?
There was a problem hiding this comment.
Great thought, I was worried about that. Do you think optionalDependencies would solve that problem??
Here is the explainer:
$ yarn explain peer-requirements pea0fd
➤ YN0000: @wordpress/e2e-tests@portal:../gutenberg/packages/e2e-tests. [62522] doesn't provide puppeteer, breaking the following requirements:
➤ YN0000: puppeteer-testing-library@npm:0.5.0 [f94aa] → * ✘It looks like puppeteer-testing-library has a peer dep requesting puppeteer:
There was a problem hiding this comment.
Great thought, I was worried about that. Do you think
optionalDependencieswould solve that problem??
I have never tried using optionalDependencies because it seems like it still installs dependencies according to https://docs.npmjs.com/cli/v8/configuring-npm/package-json#optionaldependencies. The difference seems to be that it doesn't fail the whole installation process when it is unable to satisfy this dependency for any reason.
It looks like
puppeteer-testing-libraryhas a peer dep requesting puppeteer:
This library is maintained by @kevin940726 so let's see what he thinks.
There was a problem hiding this comment.
I think we should list them as peerDependencies, but also mark them as optional using peerDependenciesMeta.
We'll deprecate puppeteer-testing-library once we migrate to Playwright, I'd recommend ignoring the warning for now if it's possible, or I can quickly publish a patch to fix that.
|
Size Change: 0 B Total Size: 1.22 MB ℹ️ View Unchanged
|
|
Thank you for another patch for better peer dependencies handling 👍🏻 I left a note about |
gziolo
added
npm Packages
gziolo
reviewed
Mar 22, 2022
noahtallen
force-pushed
the
try/add-more-peer-dependencies
branch
from
60180cf to
2bd943a
Compare
|
I also added peer deps to the preferences package. (Side note, I'm unsure about the package-lock changes! Hopefully CI works with them.) |
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.
Description
I found a few more peer dependency warnings while testing in https://github.com/noahtallen/test-yarn-gutenberg/. I think this should be the final update needed to get all peer dependencies resolved among WordPress dependencies. The remaining errors are related to 3rd party packages, from what I can tell.
Specifically:
postcssto one of the postcss plugins.Testing Instructions
CI
Screenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).