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

Replace react-virtualized with react-virtuoso for React 18 compatibility #11553

Merged
merged 7 commits into from
Aug 16, 2022

Conversation

msujew
Copy link
Member

@msujew msujew commented Aug 10, 2022

What it does

Fixes a problem which came up during the React 18 upgrade (#11455). Removes the need for the react resolutions and replaces the outdated react-virtualized package with the react-virtuoso package. Although the react-window package is a spiritual successor to the react-virtualized dependency, it's missing many features which were needed for our tree rendering pipeline (namely measuring elements and caching these measurements).

Also fixes #11376 accidentally in the process. I just wanted to test the "endless list" option of the react-virtuoso package.

Also aligns the scrolling behavior of trees to vscode (it always scrolls selected elements into the center if they aren't visible).

How to test

  1. Perform manual smoke tests on tree widgets throughout Theia. There shouldn't be any regressions compared to the use of react-virtualized.
  2. Open the History view and assert that the view only loads a hundred commits at a time (and more once you reach the bottom of the list).

Review checklist

Reminder for reviewers

@msujew msujew added the tree issues related to the tree (ex: tree widget) label Aug 10, 2022
@msujew msujew force-pushed the msujew/react-virtuoso branch from 55a4acf to efcb4ec Compare August 10, 2022 15:36
@msujew msujew force-pushed the msujew/react-virtuoso branch from c8ea3e2 to 6690e11 Compare August 11, 2022 11:47
@colin-grant-work colin-grant-work self-requested a review August 15, 2022 17:27
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

Smoke tests look good 👍. I think I'd be inclined to merge sooner rather than later to give more time to catch any bugs.

@colin-grant-work
Copy link
Contributor

(Though I'd also like CI to run and pass...)

@msujew
Copy link
Member Author

msujew commented Aug 15, 2022

@colin-grant-work I'll rebase, but the 3PP dash license CI fails anyway, since the @virtuoso.dev/urx package license cannot be detected correctly by the check script. It somehow believes the package to be named just virtuoso.dev. See here and this due diligence issue. I confirmed that it's MIT, is it alright to put it in the baseline json manually? (see also the clearlydefined entry for the package) @vince-fugnitto any idea?

@msujew msujew force-pushed the msujew/react-virtuoso branch from 383b72f to ec2feee Compare August 15, 2022 21:12
@vince-fugnitto
Copy link
Member

@msujew it is probably fine to add it to our baseline for now given that the tool itself cannot parse the package's name. I do not see results for the package at that version on clearlydefined so we could also wait to see what Wayne says.

@msujew
Copy link
Member Author

msujew commented Aug 16, 2022

@vince-fugnitto Yeah I also had troubles finding it on clearlydefined. But adjusting the URL did the trick for me and it shows as MIT only. See here.

@msujew msujew merged commit a58e2f4 into master Aug 16, 2022
@msujew msujew deleted the msujew/react-virtuoso branch August 16, 2022 13:29
@github-actions github-actions bot added this to the 1.29.0 milestone Aug 16, 2022
@anavarre
Copy link

anavarre commented Oct 6, 2022

Also fixes #11376 accidentally in the process. I just wanted to test the "endless list" option of the react-virtuoso package.

I know this has long been merged but with 1.29 I'm still very much seeing the max count is not respected. Should we reopen #11376?

2022-10-06 09 37 51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tree issues related to the tree (ex: tree widget)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scm: history does not respect max count
4 participants