Skip to content

fix(package.json): fixes issues people were having with react 17 and react-sortable-tree #80

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

benatshippabo
Copy link

@benatshippabo benatshippabo commented Jan 22, 2021

Issues

Many people are having issues with react-sortable-tree after upgrading to React 17. It looks like the cause is using findDOMNode and mismatched React versions due to a peer dependency issue. This PR should resolve the issues.

facebook/react#20131 (comment)
frontend-collective/react-sortable-tree#821
frontend-collective/react-sortable-tree#450
frontend-collective/react-sortable-tree#281 (comment)
#70

Test steps

  1. yarn
  2. yarn build
  3. in the react-sortable-tree repo
  4. yarn add file:$PATH_TO_THIS_REPO/lib
  5. add in some yarn resolutions in package.json so storybook will use React 17
"resolutions": {
    "react": "^17.0.1",
    "react-dom": "^17.0.1"
}
  1. yarn storybook
  2. test around make sure nothing is broken

@benatshippabo
Copy link
Author

@wuweiweiwu @fritz-c could you take a look at this PR please?

@omnidan
Copy link

omnidan commented Feb 2, 2021

@wuweiweiwu would it be possible to make a new release with this PR merged? this is currently breaking on all projects using the latest react version

@JacobSoderblom
Copy link

Any updates on this? We are blocked as well in our project.

@naiaramartin
Copy link

naiaramartin commented Mar 5, 2021

We have tested it using patch-package and can confirm it works on React 17

@alyavasilyeva
Copy link

hey, is there any hope the PR will be merged this month? reeeeally need this fix :)

@benatshippabo
Copy link
Author

@wuweiweiwu @fritz-c any progress on reviewing this pr?

@dottoreFell
Copy link

dottoreFell commented Apr 19, 2021

Total newbie here. Could anyone give me simple directions on how to apply patches and use this repo as module in React?

@benatshippabo
Copy link
Author

Total newbie here. Could anyone give me simple directions on how to apply patches and use this repo as module in React?

  1. Go to the file you want to update in your node_modules folder.
  2. Make the changes you desire and save.
  3. Generate the patches with the patch-package npm package.
  4. Make sure you call patch-package as a postinstall script.
  5. Reinstall npm packages npm install or yarn

@dottoreFell
Copy link

@benatshippabo thanks for the answer. Should I do that with version from npm or with the github version? The npm version seem to be different from this version. I tried to patch the npm version but then I was getting compilation errors:

./node_modules/frontend-collective-react-dnd-scrollzone/lib/index.js
SyntaxError: C:\Users\Michal\source\simpleEDM\simpleedm_gui_old\node_modules\frontend-collective-react-dnd-scrollzone\lib\index.js: Support for the experimental syntax 'classProperties' isn't currently enabled (61:24):

  59 | export function createScrollingComponent(WrappedComponent) {
  60 |   class ScrollingComponent extends Component {
> 61 |     static displayName = `Scrolling(${getDisplayName(WrappedComponent)})`;
     |                        ^
  62 |
  63 |     static propTypes = {
  64 |       // eslint-disable-next-line react/forbid-prop-types

Add @babel/plugin-proposal-class-properties (https://git.io/vb4SL) to the 'plugins' section of your Babel config to enable transformation.
If you want to leave it as-is, add @babel/plugin-syntax-class-properties (https://git.io/vb4yQ) to the 'plugins' section to enable parsing.

Is it normal behavior after patching and I should change babel configuration?

@benatshippabo
Copy link
Author

Is it normal behavior after patching and I should change babel configuration?

@dottoreFell Definitely not normal behavior 🤔 , probably just install and list the babel plugin

@dottoreFell
Copy link

Sorry, but I am not sure how to understand it. Should I install https://www.npmjs.com/package/babel-plugin ? It seems that I know too little about packages and wasting Your time. It's not likely, but maybe some day one of the maintainers will have time and mercy and will just merge this patch :D

@benatshippabo
Copy link
Author

@wuweiweiwu @fritz-c if everything looks good, could you please merge this?

@ThaddeusJiang
Copy link

waiting for this 🙏

@LuisAlejandro
Copy link

Please merge

@michal-grzelak
Copy link

@wuweiweiwu @fritz-c Hi, all necessary work was done in this PR. Could You please merge it? There are many people waiting for that 🙂

@dhavyd
Copy link

dhavyd commented May 5, 2021

Please merge :-)

@dhavyd
Copy link

dhavyd commented May 6, 2021

Interesting, I have tested a lot, removing node_modules, yarn.lock and package-lock.json several times... When I install with npm, using React 17, I get that react-sortable-tree requires React 16. I f I use npm install --force then it's installed, but I get the following error in my app

Screenshot 2021-05-06 at 15 40 54

If I use yarn install instead, then it works!

Note that I'm using isVirtualized={false} in both cases.

@matyas-igor
Copy link

@wuweiweiwu @fritz-c please merge when possible, waiting for these fixes as well! 😀

@bentron2000
Copy link

Adding my bit - please merge :)

@kmvan
Copy link

kmvan commented May 20, 2021

Why nobody merge?

@JacquesBosman JacquesBosman mentioned this pull request May 9, 2022
@sorinpav
Copy link

Could anyone please merge this? Thank you!

It appears that there are no conflicts with the base branch either, it just needs anyone with write access to this repo. Thanks again!

@kmvan
Copy link

kmvan commented May 19, 2022

Could anyone please merge this? Thank you!

It appears that there are no conflicts with the base branch either, it just needs anyone with write access to this repo. Thanks again!

The project is dead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.