emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.5k stars 1.11k forks source link

Frontity with @emotion/react pulls in incorrect react version #2273

Open mleongtt opened 3 years ago

mleongtt commented 3 years ago

Current behavior:

When upgrading frontity to the latest version, it uses react 17.0.*. When running if for local development, it errors out telling us there is probably multiple versions of react/react-dom/. One of its dependencies are @emotion/react, which is pulling in the devDependency "react": "16.14.0" but in peerDependency, its "react": ">=16.8.0".

Is there a reason devDependency is fixed to a specific version or can it be adjusted like peerDependency?

Andarist commented 3 years ago

dev dependencies are not downloaded when a package is used by another one so this doesn't matter

mleongtt commented 3 years ago

as im tracing the node modules [myapp]/node_modules/@emotion/react/node_modules/react/package.json it tells me "version": "16.14.0". Do you know why its installing that one then?

Andarist commented 3 years ago

Hard to tell - having @emotion/react as a dependency should not install any react version.

Please always try to share a repro case in a runnable form - either by providing a git repository to clone or a codesandbox. OSS maintainers usually can't afford the time to set up the repro, even if exact steps are given.

SimeonC commented 3 years ago

I get the same error using npm 7 locally. I don't know Frontity, but I'm guessing it's using the most up to date version of node/npm. npm 7 started automatically trying to install and resolve peerDependencies, if there is a conflict in peer deps (even if you have it installed directly) it throws an error and fails to install.

I think the easy short fix is to change "react": ">=16.8.0" to "react": ">=16.8.0 || ^17". (Annoyingly >=16 doesn't include 17+ in npm semver)

NPM Version; 7.5.3 Node Version; v15.9.0

> npm i
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! Found: react@17.0.1
npm ERR! node_modules/react
npm ERR!   react@"17.0.1" from the root project
npm ERR!   peer react@">=16.8.0" from @emotion/react@11.1.5
npm ERR!   node_modules/@emotion/react
npm ERR!     @emotion/react@"11.1.5" from the root project
npm ERR!     peer @emotion/react@"^11.0.0-rc.0" from @emotion/styled@11.0.0
npm ERR!     node_modules/@emotion/styled
npm ERR!       @emotion/styled@"11.0.0" from the root project

EDIT: FYI the workaround is to use the --legacy-peer-deps flag, npm install --legacy-peer-deps. Don't know if this helps you in Frontity, but in normal NPM this works.

Andarist commented 3 years ago

Annoyingly >=16 doesn't include 17+ in npm semver

This is somewhat weird as this holds true:

require('semver').satisfies('17.0.0', '>=16')

Maybe npm uses a different logic there but usually they use the semver package which is maintained by npm's creator - Issac.

Either way - if changing the peer dep range in a way that you have described it solves the problem then I don't see why we wouldn't change it. Could you confirm it somehow?

SimeonC commented 3 years ago

Yea, dunno about that, they might have changed. Anyway, I had a similar request on another library here which fixed a similar problem if that’s enough confirmation ; https://github.com/react-ga/react-ga/pull/461

Andarist commented 3 years ago

That has changed a different type of dependency range though (^).

Would you be able to prepare a repro case of this? I would love to jump into this to check out a few things.

SimeonC commented 3 years ago

Sure I can probably try to find some time tomorrow. Though you can test it on any project with @emotion/react in it. Just install node 15 (or npm 7, node 15 bundles npm 7 so this is where most people will hit the issue), delete package-lock + node_modules, run npm i and it should error.

SimeonC commented 3 years ago

Huh, I tried to replicate this morning and I think I was reading the error wrong. The following package.json installs correctly with npm 7 indicating that it's not emotion's fault.

{
  "name": "test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "@emotion/react": "11.1.5",
    "react": "17.0.1"
  }
}

For reference, here's the screenshot of my error - which clearly says it's not emotion's fault if I kept reading 🤦 . The error should be read;

CleanShot 2021-03-15 at 16 06 16

@mleongtt with frontity are you seeing an error similar to that?

mleongtt commented 3 years ago

"version": "1.13.0",