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

fix(deps) resolve yarn warnings regarding unmet peer dependencies for Redwood projects #8874

Merged
merged 14 commits into from
Feb 8, 2024

Conversation

thedavidprice
Copy link
Contributor

@thedavidprice thedavidprice commented Jul 10, 2023

Running yarn install in a Redwood project results in a wall of information, most of which are warnings about peer dependencies and unmet dependency version requirements. We (framework authors) put up with it but that's not the case for developers — best case they find the output warnings annoying (and hopefully don't miss important errors); worst case it turns people away from using Redwood.

Some, but not all, warnings are from Redwood packages. Many are upstream. At this time, there is no known negative impact from these dependency warnings — i.e. ideally they would all be resolved, but doing so has diminishing returns and is an ongoing maintenance consideration. This PR silences warnings that are considered acceptable and resolves those that are not. (Note: contributions are welcome for anyone who'd like to completely resolve these warnings!)

Lastly, warnings != errors. This PR does not touch errors (if/when applicable).

Resulting Output

Here's what yarn install will look like with changes from this PR

yarn-install

Nice, right?!? 🤯

Handling existing peer dep warnings

There are two cases of warnings handled by this PR.

Case 1) YN0002 - MISSING_PEER_DEPENDENCY
This PR silences these warnings using yarnrc logFilters.

Important: devs can still inspect these warnings via yarn explain peer-requirements or by managing their projects' logFilters config.

Case 2) YN0060 - INCOMPATIBLE_PEER_DEPENDENCY
Because this can only be resolved via the parent package, this PR moves the necessary package code into the respective Redwood Package, thus properly fixing the incompatibility.

Breaking

This PR breaks a CSS import in the a11y Template layout.tsx.a11yTemplate, which is used when anyone generates a layout component with the --skipLink command option: yarn redwood generate layout <name> --skipLink

// web/src/layouts/MyLayout/MyLayout.{jsx|tsx}

import { SkipNavLink, SkipNavContent } from '@redwoodjs/router'
-import '@reach/skip-nav/styles.css'
+import '@redwoodjs/router/skip-nav.css'

ToDo

  • Update redwoodjs/docs/a11y
  • Codemode for layout component import change (might not be necessary as find+replace would work just fine)

@thedavidprice thedavidprice added the release:breaking This PR is a breaking change label Jul 10, 2023
@thedavidprice thedavidprice added this to the v6.0.0 milestone Jul 10, 2023
@thedavidprice thedavidprice requested a review from jtoar July 10, 2023 23:44
@jtoar jtoar modified the milestones: v6.0.0, next-release Jul 27, 2023
@jtoar jtoar modified the milestones: next-release, v6.1.0 Aug 12, 2023
@thedavidprice
Copy link
Contributor Author

@jtoar any chance to get this in given that you agree with me there's no need to create a codemod here:

  • there are very instances of the a11y generator being used
  • a simple find/replace works very well in this case

What say you?

@jtoar jtoar modified the milestones: next-release, v6.2.0 Sep 2, 2023
@jtoar jtoar modified the milestones: next-release, v6.3.0 Sep 16, 2023
@Tobbe Tobbe self-assigned this Dec 17, 2023
@thedavidprice
Copy link
Contributor Author

@Tobbe can we target this one for v7? It's a nice DX + quality of life change. Already documented for Upgrade Guide.

Happy to handle conflicts. Just let me know.

@Tobbe
Copy link
Member

Tobbe commented Dec 18, 2023

@thedavidprice I agree. Looks like a really nice DX improvement. Dom had some reservations about exactly how it was implemented. He'll review and get back to us with comments.

@jtoar
Copy link
Contributor

jtoar commented Dec 18, 2023

Important

A lot of these can be fixed by just adding the missing dependency. Since we enforce that all our packages depend on the same version of a dependency, as long as one of our packages already depends on the missing dependency, "adding" a dependency to a package doesn't incur an increase in size.

I'd like to try to fix the warnings instead of silencing them. I understand that some of them aren't our packages and will have to be fixed upstream, in which case silencing may be the answer. But most of them are ours. Here's the errors we get (on CRWA install):

➤ YN0002: │ @graphql-tools/graphql-tag-pluck@npm:7.5.2 [c7cbc] doesn't provide @babel/core (p1e703), requested by @babel/plugin-syntax-import-assertions
➤ YN0002: │ @redwoodjs/babel-config@npm:6.5.1 doesn't provide graphql-tag (pf22c7), requested by babel-plugin-graphql-tag
➤ YN0002: │ @redwoodjs/cli@npm:6.5.1 doesn't provide react (pe6864), requested by @redwoodjs/prerender
➤ YN0002: │ @redwoodjs/cli@npm:6.5.1 doesn't provide react-dom (pa48b5), requested by @redwoodjs/prerender
➤ YN0002: │ @redwoodjs/core@npm:6.5.1 doesn't provide @babel/core (pe4eb4), requested by @babel/cli
➤ YN0002: │ @redwoodjs/core@npm:6.5.1 doesn't provide @babel/core (p46f04), requested by babel-loader
➤ YN0002: │ @redwoodjs/core@npm:6.5.1 doesn't provide graphql (pe91e0), requested by graphql-tag
➤ YN0002: │ @redwoodjs/eslint-config@npm:6.5.1 doesn't provide babel-plugin-module-resolver (p80f0a), requested by eslint-import-resolver-babel-module
➤ YN0002: │ @redwoodjs/internal@npm:6.5.1 doesn't provide @babel/core (pf8803), requested by @babel/plugin-transform-react-jsx
➤ YN0002: │ @redwoodjs/internal@npm:6.5.1 doesn't provide @babel/core (p0e01e), requested by @babel/plugin-transform-typescript
➤ YN0002: │ @redwoodjs/internal@npm:6.5.1 doesn't provide @types/node (pa9a45), requested by ts-node
➤ YN0002: │ @redwoodjs/internal@npm:6.5.1 doesn't provide graphql-tag (p760e6), requested by @graphql-codegen/typescript-react-apollo
➤ YN0002: │ @redwoodjs/prerender@npm:6.5.1 [6c4a1] doesn't provide prop-types (p735c2), requested by @redwoodjs/web
➤ YN0002: │ @redwoodjs/testing@npm:6.5.1 doesn't provide @babel/core (pf0dd0), requested by babel-jest
➤ YN0002: │ @redwoodjs/testing@npm:6.5.1 doesn't provide @testing-library/dom (p2a890), requested by @testing-library/user-event
➤ YN0002: │ @redwoodjs/testing@npm:6.5.1 doesn't provide prop-types (p36610), requested by @redwoodjs/web
➤ YN0002: │ @redwoodjs/testing@npm:6.5.1 doesn't provide react (p51fa6), requested by @redwoodjs/router
➤ YN0002: │ @redwoodjs/testing@npm:6.5.1 doesn't provide react (pc9a52), requested by @redwoodjs/web
➤ YN0002: │ @redwoodjs/testing@npm:6.5.1 doesn't provide react (p82884), requested by @testing-library/react
➤ YN0002: │ @redwoodjs/testing@npm:6.5.1 doesn't provide react-dom (p99887), requested by @redwoodjs/router
➤ YN0002: │ @redwoodjs/testing@npm:6.5.1 doesn't provide react-dom (pafa91), requested by @redwoodjs/web
➤ YN0002: │ @redwoodjs/testing@npm:6.5.1 doesn't provide react-dom (pec9a5), requested by @testing-library/react
➤ YN0002: │ react-hot-toast@npm:2.4.1 [7fc6b] doesn't provide csstype (pfadc9), requested by goober
➤ YN0002: │ react-hot-toast@npm:2.4.1 [83bf0] doesn't provide csstype (pe3290), requested by goober
➤ YN0002: │ react-hot-toast@npm:2.4.1 [ca55a] doesn't provide csstype (pd2f5d), requested by goober
➤ YN0002: │ web@workspace:web doesn't provide graphql (paad35), requested by @redwoodjs/forms
➤ YN0060: │ web@workspace:web provides react (p993b3) with version 18.2.0, which doesn't satisfy what @redwoodjs/router and some of its descendants request
➤ YN0060: │ web@workspace:web provides react-dom (p76ae6) with version 18.2.0, which doesn't satisfy what @redwoodjs/router and some of its descendants request

From top to bottom:

  • [BLOCKED] @graphql-tools/graphql-tag-pluck

    ➤ YN0002: │ @graphql-tools/graphql-tag-pluck@npm:7.5.2 [c7cbc] doesn't provide @babel/core (p1e703), requested by @babel/plugin-syntax-import-assertions
    

    Depends on the @graphql-codegen major upgrade, the status of which is unknown.

  • @redwoodjs/babel-config

    ➤ YN0002: │ @redwoodjs/babel-config@npm:6.5.1 doesn't provide graphql-tag (pf22c7), requested by babel-plugin-graphql-tag
    

    Fixed by adding graphql-tag.

  • @redwoodjs/cli

    ➤ YN0002: │ @redwoodjs/cli@npm:6.5.1 doesn't provide react (pe6864), requested by @redwoodjs/prerender
    ➤ YN0002: │ @redwoodjs/cli@npm:6.5.1 doesn't provide react-dom (pa48b5), requested by @redwoodjs/prerender
    

    Let's just remove the peer dependency on react, react-dom; CLI will never provide it. It'll be provided by web, but the CLI won't be installed in the web workspace so it doesn't make sense.

  • @redwoodjs/core

    ➤ YN0002: │ @redwoodjs/core@npm:6.5.1 doesn't provide @babel/core (pe4eb4), requested by @babel/cli
    ➤ YN0002: │ @redwoodjs/core@npm:6.5.1 doesn't provide @babel/core (p46f04), requested by babel-loader
    ➤ YN0002: │ @redwoodjs/core@npm:6.5.1 doesn't provide graphql (pe91e0), requested by graphql-tag
    

    Fixed by adding these packages.

  • @redwoodjs/eslint-config

    ➤ YN0002: │ @redwoodjs/eslint-config@npm:6.5.1 doesn't provide babel-plugin-module-resolver (p80f0a), requested by eslint-import-resolver-babel-module
    

    Fixed by removing this package since it seems unused.

  • @redwoodjs/prerender

    ➤ YN0002: │ @redwoodjs/prerender@npm:6.5.1 [6c4a1] doesn't provide prop-types (p735c2), requested by @redwoodjs/web
    

    This has been fixed by removing prop-types in v7.

  • [BLOCKED] react-hot-toast

    ➤ YN0002: │ react-hot-toast@npm:2.4.1 [7fc6b] doesn't provide csstype (pfadc9), requested by goober
    ➤ YN0002: │ react-hot-toast@npm:2.4.1 [83bf0] doesn't provide csstype (pe3290), requested by goober
    ➤ YN0002: │ react-hot-toast@npm:2.4.1 [ca55a] doesn't provide csstype (pd2f5d), requested by goober
    

    Not sure why this is showing up three times. We're blocked by Make csstype a dependency. timolins/react-hot-toast#262.

  • web workspace, forms

    ➤ YN0002: │ web@workspace:web doesn't provide graphql (paad35), requested by @redwoodjs/forms
    

    We should just make graphql a dependency of forms.

  • web workspace react, react-dom

    ➤ YN0060: │ web@workspace:web provides react (p993b3) with version 18.2.0, which doesn't satisfy what @redwoodjs/router and some of its descendants request
    ➤ YN0060: │ web@workspace:web provides react-dom (p76ae6) with version 18.2.0, which doesn't satisfy what @redwoodjs/router and some of its descendants request
    

    This one's been fixed by David's changes.

@jtoar jtoar marked this pull request as draft December 18, 2023 22:02
@Tobbe
Copy link
Member

Tobbe commented Dec 27, 2023

Here's an instance of where those warnings tripped up one of our users: #9760

@jtoar jtoar marked this pull request as ready for review February 8, 2024 04:56
@jtoar jtoar unassigned Tobbe Feb 8, 2024
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

Merging this one. I scaled back my changes after yarn 4. Yarn 4 drastically reduces the number of peer dependency warnings it console out of the box. From the blog post:

You will also notice it doesn't print as much warnings around peer dependencies, as we now try to only print warnings for actionable situations.

Here's the dependency warnings yarn v4 prints:

$ yarn
➤ YN0000: · Yarn 4.0.2
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed
➤ YN0000: ┌ Post-resolution validation
➤ YN0060: │ react is listed by your project with version 18.2.0, which doesn't satisfy what @redwoodjs/router (p1eae0) and other dependencies request (but they have non-overlapping ranges!).
➤ YN0060: │ react-dom is listed by your project with version 18.2.0, which doesn't satisfy what @redwoodjs/router (p62015) and other dependencies request (but they have non-overlapping ranges!).
➤ YN0002: │ web@workspace:web doesn't provide graphql (p0b33c), requested by @redwoodjs/forms.
➤ YN0086: │ Some peer dependencies are incorrectly met; run yarn explain peer-requirements <hash> for details, where <hash> is the six-letter p-prefixed code.
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 0s 397ms
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed in 0s 348ms
➤ YN0000: · Done with warnings in 0s 948ms

There's only three now. David's original changes address the react ones, and the change I made to forms gets rid of the third.

Just so that I don't get misinterpreted, this doesn't mean that everything is fixed. (Things weren't necessarily broken either, Yarn was just doing it's job.) I just want this PR to get merged because what it has is already a big win, and I've let the scope creep too much before when I tried doing too much.

@jtoar jtoar merged commit cd65bf6 into main Feb 8, 2024
40 checks passed
@jtoar jtoar deleted the dsp-resolve-yarn-warnings-peer-deps branch February 8, 2024 05:04
@jtoar jtoar modified the milestones: next-release, v7.0.0 Feb 8, 2024
jtoar added a commit that referenced this pull request Feb 8, 2024
… Redwood projects (#8874)

Running `yarn install` in a Redwood project results in a wall of
information, most of which are warnings about peer dependencies and
unmet dependency version requirements. We (framework authors) put up
with it but that's not the case for developers — best case they find the
output warnings annoying (and hopefully don't miss important errors);
worst case it turns people away from using Redwood.

Some, but not all, warnings are from Redwood packages. Many are
upstream. At this time, there is no known negative impact from these
dependency warnings — i.e. ideally they would all be resolved, but doing
so has diminishing returns and is an ongoing maintenance consideration.
This PR silences warnings that are considered acceptable and resolves
those that are not. (Note: contributions are welcome for anyone who'd
like to completely resolve these warnings!)

Lastly, warnings != errors. This PR does not touch errors (if/when
applicable).

Here's what `yarn install` will look like with changes from this PR

![yarn-install](https://github.com/redwoodjs/redwood/assets/2951/278d2f36-a5e8-430d-8179-f6a8899d3227)

Nice, right?!? 🤯

There are two cases of warnings handled by this PR.

Case 1) [YN0002 -
MISSING_PEER_DEPENDENCY](https://yarnpkg.com/advanced/error-codes#yn0002---missing_peer_dependency)
This PR silences these warnings using yarnrc `logFilters`.

Important: devs can still inspect these warnings via `yarn explain
peer-requirements` or by managing their projects' `logFilters` config.

Case 2) [YN0060 -
INCOMPATIBLE_PEER_DEPENDENCY](https://yarnpkg.com/advanced/error-codes#yn0060---incompatible_peer_dependency)
Because this can only be resolved via the parent package, this PR moves
the necessary package code into the respective Redwood Package, thus
properly fixing the incompatibility.

This PR breaks a CSS import in the a11y Template
`layout.tsx.a11yTemplate`, which is used when anyone generates a layout
component with the `--skipLink` command option: `yarn redwood generate
layout <name> --skipLink`

```diff
// web/src/layouts/MyLayout/MyLayout.{jsx|tsx}

import { SkipNavLink, SkipNavContent } from '@redwoodjs/router'
-import '@reach/skip-nav/styles.css'
+import '@redwoodjs/router/skip-nav.css'
```

- [x] Update redwoodjs/docs/a11y
- [ ] Codemode for layout component import change (might not be
necessary as find+replace would work just fine)

---------

Co-authored-by: Dominic Saadi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants