denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
98.08k stars 5.4k forks source link

`deno install` with incorrect peer dependencies installs incompatible React versions #26841

Open alexgleason opened 1 week ago

alexgleason commented 1 week ago

Version: Deno 2.0.6

So... I am migrating a legacy React Vite project from Node+Yarn to Deno.

When I run deno install followed by deno run -A npm:vite serve, the page crashes in the browser with a runtime error from React:

Error: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:
1. You might have mismatching versions of React and the renderer (such as React DOM)
2. You might be breaking the Rules of Hooks
3. You might have more than one copy of React in the same app
See https://reactjs.org/link/invalid-hook-call for tips about how to debug and fix this problem.

I have narrowed this problem down to multiple incompatible React versions being installed by deno install. It installs 3 different versions of React:

Screenshot from 2024-11-12 13-15-26

The problem is actually that I have incorrect peer dependencies in my project. When I run yarn normally, it's full of peer dependency warnings. I understand this is a problem with my project, but it's not easy to fix. It's caused by some of my libraries having a peer-dependency on React 16 or 17, while my project has been upgraded to React 18 (which yarn allowed me to do).

image

If I use yarn to set up node_modules, and then use deno run -A npm:vite serve, it all works perfectly. So the problem is related to the behavior of deno install and not the Deno runtime itself.

As for the solution, I think Deno should at least warn about peer dependencies. I am not sure if there's a standard expected behavior among package managers about what to do in this scenario, or if there are any workarounds besides replacing these unmaintained packages with forks. (EDIT: I tried specifying overrides to react 18 in package.json and it had no effect.)

For reference, this is the project: https://gitlab.com/soapbox-pub/soapbox

(I feel bad that Deno is burdened by this legacy Node crap, but I thought you would rather know than not know. :smiling_face_with_tear:)

nathanwhit commented 1 week ago

Hmm I think this falls out of the way that we autoinstall peer dependencies. Maybe we shouldn't do the autoinstall if the user has a version of the peer dep already installed at the top level, and instead warn if the version is wrong (like yarn/pnpm does)

alexgleason commented 1 week ago

I think that's a good idea @nathanwhit

To be sure, I re-read the definition of peerDependencies:

Trying to install another plugin with a conflicting requirement may cause an error if the tree cannot be resolved correctly.

Thinking about this logically, it suggests to me that only one version of the package is meant to be installed. Whether to throw an error (or warn and take one of the versions) is an implementation detail of the package manager.

In fact, I tried npm install of the very same project and it does indeed throw an error!

image

But to get around it, I can use npm install --force, which works. It does the same thing as yarn and installs only the version specified in my package.json (React 18 in this case).

I think either of these solutions would be good. I have a slight preference towards the way yarn does it, to warn instead of requiring a --force flag. But I am biased because my project is already broken. :sweat_smile:

Now that I understand fully, I realized this issue is mostly a duplicate of https://github.com/denoland/deno/issues/26145

https://github.com/denoland/deno/issues/20614 https://github.com/denoland/deno/issues/17286 are also related.