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

netlify-cms-app fails to install with react 17 as version 6 of react-aria-menubutton in incompatible with react 17 #5376

Closed
something-something-something opened this issue May 12, 2021 · 20 comments
Labels
priority: high type: bug code to address defects in shipped code

Comments

@something-something-something
Copy link
Contributor

Describe the bug

When installing netlify-cms-app with react 17 the install fails with the following error:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! Found: [email protected]
npm ERR! node_modules/react
npm ERR!   peer react@"17.0.2" from [email protected]
npm ERR!   node_modules/react-dom
npm ERR!     react-dom@"^17.0.2" from the root project
npm ERR!     peer react-dom@"^16.8.4 || ^17.0.0" from [email protected]
npm ERR!     node_modules/netlify-cms-app
npm ERR!       netlify-cms-app@"*" from the root project
npm ERR!     8 more (netlify-cms-widget-date, ...)
npm ERR!   react@"^17.0.2" from the root project
npm ERR!   38 more (netlify-cms-app, @emotion/core, @emotion/styled, ...)
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer react@"0.14.x || ^15.0.0 || ^16.0.0" from [email protected]
npm ERR! node_modules/netlify-cms-ui-default/node_modules/react-aria-menubutton
npm ERR!   react-aria-menubutton@"^6.0.0" from [email protected]
npm ERR!   node_modules/netlify-cms-ui-default
npm ERR!     netlify-cms-ui-default@"^2.13.1" from [email protected]
npm ERR!     node_modules/netlify-cms-app
npm ERR!       netlify-cms-app@"*" from the root project
npm ERR!     23 more (netlify-cms-backend-azure, ...)
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!

It seems like react-aria-menubutton in netlify-cms-ui-default needs to be upgraded to version 7.0.1 see: davidtheclark/react-aria-menubutton#140

To Reproduce

Run npm init -y && npm i react react-dom && npm i netlify-cms-app in an empty directory.
See Error

Expected behavior

netlify-cms-app installs without having to use --legacy-peer-deps

Screenshots

NA

Applicable Versions:

  • Netlify CMS version: 2.15.6
  • Git provider: NA
  • OS: Arch Linux
  • Browser version NA
  • NPM version: 7.12.1
  • Node.JS version: 16.1.0

CMS configuration

NA

Additional context

Seems like a dependency that didn't get updated while fixing
#5111

@something-something-something something-something-something added the type: bug code to address defects in shipped code label May 12, 2021
@erezrokah
Copy link
Contributor

erezrokah commented May 13, 2021

@erezrokah
Copy link
Contributor

Blocked by davidtheclark/react-aria-menubutton#133

@shirshendubhowmick
Copy link

Blocked by davidtheclark/react-aria-menubutton#133

This is now complete

@erezrokah
Copy link
Contributor

Woo @shirshendubhowmick, this is amazing. Thank you for the quick follow up ❤️

@paulobarcelos
Copy link

paulobarcelos commented May 26, 2021

It seems the react-codemirror nested dependency could also be blocking this?
This is the error I get when trying to update to React 17:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! Found: [email protected]
npm ERR! node_modules/react
npm ERR!   react@"^17.0.2" from the root project
npm ERR!   peer react@">=16.8 || ^17.0.0" from [email protected]
npm ERR!   node_modules/framer-motion
npm ERR!     framer-motion@"^4.1.17" from the root project
npm ERR!   45 more (react-awesome-styled-grid, react-dnd, react-dom, ...)
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer react@">=15.5 <=16.x" from [email protected]
npm ERR! node_modules/netlify-cms-widget-code/node_modules/react-codemirror2
npm ERR!   react-codemirror2@"^7.0.0" from [email protected]
npm ERR!   node_modules/netlify-cms-widget-code
npm ERR!     netlify-cms-widget-code@"^1.3.2" from [email protected]
npm ERR!     node_modules/netlify-cms-app
npm ERR!       netlify-cms-app@"^2.15.11" from the root project
npm ERR!

@erezrokah
Copy link
Contributor

Thanks @paulobarcelos, that's helpful. Looks like there's a PR for it scniro/react-codemirror2#224, but no comment from the maintainer.

@paulobarcelos
Copy link

paulobarcelos commented May 26, 2021

Thanks @paulobarcelos, that's helpful. Looks like there's a PR for it scniro/react-codemirror2#224, but no comment from the maintainer.

@erezrokah, by the looks of it, seems that project may be dead. Wonder if it would be a good idea for Netlifly CMS to change to the proposed updated fork while the original one doesn't change (if it ever will). Feels like halting support for React 17 is a bigger issue than dealing with the annoyance of having to migrate to an unofficial fork of the dependency.

@erezrokah
Copy link
Contributor

That fork doesn't seem to be updating frequently too and I don't think we want to install directly from GitHub.

For React 17 + npm@7 support there's a workaround by passing the --legacy-peer-deps flag, which is clearly stated in the error.

Not sure if we should find a completely different solution to our Code widget.

Since this seems to be an issue with more that one dependency, we might have to think of a solution that doesn't involve chasing down maintainers.

I wonder if there's a way we can update dependencies package.json files on the CMS side to allow users to install it.

@LuisOsta
Copy link

LuisOsta commented Nov 9, 2021

I get a similar error when trying to install the package netlify-cms-app:

npm ERR! Could not resolve dependency:
npm ERR! peer react@"^16.0.0-0" from [email protected]
npm ERR! node_modules/netlify-cms-app/node_modules/react-split-pane
npm ERR!   react-split-pane@"^0.1.85" from [email protected]
npm ERR!   node_modules/netlify-cms-app/node_modules/netlify-cms-core
npm ERR!     netlify-cms-core@"^2.54.0" from [email protected]
npm ERR!     node_modules/netlify-cms-app
npm ERR!       netlify-cms-app@"*" from the root project
npm ERR! 

It seems that react-split-plane may be dead or at least poorly maintained as the last commit was in May. There's a highly commented issue for the react 17 upgrade - tomkp/react-split-pane#713 and a PR for it - tomkp/react-split-pane#681. With the maintainer having left no comments on either.

Is there a way we can move forward without using --legacy-peer-deps, as that often causes its own headaches that I'd love to avoid

@JeremyGrieshop
Copy link

It seems that react-split-plane may be dead or at least poorly maintained as the last commit was in May. There's a highly commented issue for the react 17 upgrade - tomkp/react-split-pane#713 and a PR for it - tomkp/react-split-pane#681. With the maintainer having left no comments on either.

After waiting for months, I realized the whole project was only a couple of MIT-licensed files, so I just copied them into my project and was done with it.

@LuisOsta
Copy link

@JeremyGrieshop How did you integrate the copied files into the package?

@JeremyGrieshop
Copy link

@JeremyGrieshop How did you integrate the copied files into the package?

I don't have a public repository to share at this moment, but I just copied these 4 files into a local directory, called SplitPane in my project and imported from it instead of react-split-pane:

https://github.com/tomkp/react-split-pane/tree/master/src

It does, at this time, require a polyfil, so you'll need to add react-lifecycles-compat as a dependency for now. If these are ever converted over to class-less components/hooks, then it won't be necessary. It also required react-style-proptype, but I just removed that code from the appropriate classes, since I didn't care about the prop types. Otherwise, you'd need to add that dependency as well.

@LuisOsta
Copy link

LuisOsta commented Jan 15, 2022

@erezrokah would it be possible to follow what this user had commented on the PR in react-split-pane - tomkp/react-split-pane#681 (comment) and simply copy the needed components into the internals of Netlify CMS with the right license?

@JeremyGrieshop
Copy link

Here's an example I quickly pushed to my personal repository that someone could copy into their own SplitPane folder. I converted over some of the older class dependencies and made them functional components that use hooks instead:

https://github.com/JeremyGrieshop/react-split-pane

@erezrokah
Copy link
Contributor

erezrokah commented Jan 17, 2022

Hi everyone 👋 At this point I think we will accept a contribution that inlines react-split-pane as it seems there won't be a any progress with tomkp/react-split-pane#681

@LuisOsta
Copy link

@erezrokah Sounds great! I'm working on that right now in this fork https://github.com/LuisOsta/netlify-cms based on the version created by @JeremyGrieshop.

Btw Jeremy let me know if the attribution I included in the commit looks good to you (I included the README and a link to your repository)

@LuisOsta
Copy link

@erezrokah If you could take a look when you have the time that'd be much appreciated

@LuisOsta
Copy link

Aside from react-split-pane are there any remaining packages that are causing React 17 compatibility issue? What's left for us to be able to definitely close this issue?

@JeremyGrieshop
Copy link

JeremyGrieshop commented Jan 17, 2022

Btw Jeremy let me know if the attribution I included in the commit looks good to you (I included the README and a link to your repository)

Looks great! I hope you find it useful and that I didn't leave anything out of the original work.

@martinjagodic
Copy link
Member

This seems to be fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants