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(remix-dev): fix config call when using Yarn PnP #3566

Closed
wants to merge 7 commits into from

Conversation

lensbart
Copy link
Contributor

@lensbart lensbart commented Jun 24, 2022

Related to #683 (already closed)

  • Lists react and react-dom as peer dependencies for all packages that consume @remix-run/server-runtime and don’t already have react(-dom) as a dependency, because @remix-run/server-runtime itself lists react(-dom) as peer dependencies. This prevents the following warning when running yarn install:
    ➤ YN0002: │ @remix-run/deno doesn't provide react (p1dd1e), requested by @remix-run/server-runtime
    ➤ YN0002: │ @remix-run/deno doesn't provide react-dom (p56944), requested by @remix-run/server-runtime
    ➤ YN0002: │ @remix-run/dev [bc263] doesn't provide react (pbd2ca), requested by @remix-run/server-runtime
    ➤ YN0002: │ @remix-run/dev [bc263] doesn't provide react-dom (pb7dc6), requested by @remix-run/server-runtime
    
  • Silences harmless error when yarn config get @remix-run:registry throws because the setting can’t be found

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jun 24, 2022

Hi @lensbart,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jun 24, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@lensbart lensbart changed the title Various minor Yarn PnP fixes Yarn PnP fixes Jun 24, 2022
@lensbart lensbart changed the title Yarn PnP fixes Fix: Yarn PnP Jun 24, 2022
@lensbart lensbart marked this pull request as ready for review June 24, 2022 21:02
@lensbart lensbart mentioned this pull request Jun 24, 2022
@MichaelDeBoey MichaelDeBoey changed the title Fix: Yarn PnP fix(remix-dev): fix config call when using Yarn PnP Jun 24, 2022
Comment on lines 24 to 26
"@cloudflare/workers-types": "^2.0.0 || ^3.0.0",
"react": ">=16.8",
"react-dom": ">=16.8"
Copy link
Member

@MichaelDeBoey MichaelDeBoey Jun 24, 2022

Choose a reason for hiding this comment

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

We already had a couple of PRs doing this and we decided we don't want this as these aren't necessary in any way.
This is only there because we're relying on react-router, which has these in peerDependencies as well.

Once we're switching to @remix-run/router (once react-router v6.4 is released), these won't be necessary anymore in peerDependencies, so all problems will be gone as well.

Suggested change
"@cloudflare/workers-types": "^2.0.0 || ^3.0.0",
"react": ">=16.8",
"react-dom": ">=16.8"
"@cloudflare/workers-types": "^2.0.0 || ^3.0.0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for that context. The desired result (no warning when using Yarn) can also be achieved by setting

  "peerDependenciesMeta": {
    "react": {
      "optional": true
    },
    "react-dom": {
      "optional": true
    }
  }

in the package.json of remix-server-runtime. I’ve made this change now, let me know if you disagree

Copy link
Member

Choose a reason for hiding this comment

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

Since it's only a warning, I think we can just leave it as-is
The warning will disappear automatically once we made the switch to @remix-run/router.

Copy link
Contributor Author

@lensbart lensbart Jun 25, 2022

Choose a reason for hiding this comment

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

While this might be fixed in the future, I’d still err on the side of correctness in the meantime.

This is a warning when running yarn install, but an error (via @yarnpkg/esbuild-plugin-pnp) when running yarn build:
Screenshot 2022-06-25 at 17 41 40

So I think the minimum necessary change is to include react and react-dom as peer dependencies of remix-dev, if nothing else. I think it’s safer to keep the peerDependenciesMeta setting because I’m concerned there are configurations that I haven’t tested that might otherwise fail like the example above.

If you think there are alternative ways to solve the error above, please let me know and I’d be happy to adapt my proposed fix accordingly.

packages/remix-deno/package.json Outdated Show resolved Hide resolved
packages/remix-dev/package.json Show resolved Hide resolved
packages/remix-node/package.json Outdated Show resolved Hide resolved
packages/remix-dev/cli/create.ts Outdated Show resolved Hide resolved
@lensbart
Copy link
Contributor Author

Closing in favour of #3594

@lensbart lensbart closed this Jun 28, 2022
@MichaelDeBoey MichaelDeBoey added the duplicate This issue or pull request already exists label Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed duplicate This issue or pull request already exists package:dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants