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.

Sign up for GitHub

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

Jump to bottom

Update React to v18 #11455

Merged
merged 5 commits into from
Aug 9, 2022
Merged

Update React to v18 #11455

merged 5 commits into from
Aug 9, 2022

Conversation

AlexandraBuzila
Copy link
Contributor

What it does

This PR updates Theia to React v18. We adapted the invocations of React where necessary and made sure that Theia does not behave differently. With these changes in place we could not observe any regressions.

The major change in React v18 affecting the Theia codebase is the deprecation of the ReactDOM.render API. Although it is still available, we adapted all invocations of React to use the new createRoot API, enabling new React v18 features like concurrent rendering throughout the code base. What we avoided is to explicitly use any of the new features to improve the Theia UX. This should be done separately.

With the removal of the render API we lost the option to use its render callbacks to trigger functionality in Theia. In consequence, the onRender field from ReactDialog and ReactWidget was removed. Users of the render callbacks are adapted to use the equivalent ref callbacks or useEffect patterns instead.

Other minor updates:

  • the children prop now needs to be explicitly specified when needed
  • React state updates are now batched (SearchInWorkspaceInput was using an intermediate state, this is fixed now)

Consumer code will most likely continue to work, especially when using ReactDOM.render for now instead of the new API, as the new features of React are then not used. In extreme cases, React 17 could be additionally loaded and used for their own code.

If a jump directly to v18 is not desired at the moment, we also prepared a branch which updates React to v17: https://github.com/eclipsesource/theia/tree/react-v17-update

Review checklist

Reminder for reviewers

Contributed on behalf of STMicroelectronics
Signed-off-by: Alexandra Buzila abuzila@eclipsesource.com

@vince-fugnitto vince-fugnitto added react issues related to the react language dependencies pull requests that update a dependency file labels Jul 25, 2022
vince-fugnitto
vince-fugnitto reviewed Jul 26, 2022
View reviewed changes
dependency-check-baseline.json Outdated Show resolved Hide resolved
package.json
Comment on lines +11 to +12
"@types/react": "18.0.15",
"@types/react-dom": "18.0.6"
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify why a resolution on the repo is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed because of react-virtualized, their latest release still uses React 16. Without the resolution block, type errors are shown e.g. 'List' cannot be used as a JSX component, similar to bvaughn/react-virtualized#1739

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of resolutions since it does not solve the same problem for downstream extensions and applications. We should probably think of using react-window instead of react-virtualized which is written by the same author and is a replacement for the latter (related). We can tackle it later if needed but we should probably mention something in the migration guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note in the migration guide.

@AlexandraBuzila
Copy link
Contributor Author

I rebased the changes on the latest master and fixed the conflicts.

msujew
msujew reviewed Aug 1, 2022
View reviewed changes
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I'd generally try to merge this relatively early in this month's dev cycle to find possible regressions and also address the outstanding comment from Vince (replacing the react-virtualized package).

packages/plugin-ext/src/main/browser/comments/comment-thread-widget.tsx Outdated Show resolved Hide resolved
packages/preferences/src/browser/views/preference-searchbar-widget.tsx Show resolved Hide resolved
packages/search-in-workspace/src/browser/search-in-workspace-widget.tsx Show resolved Hide resolved
msujew
msujew approved these changes Aug 2, 2022
View reviewed changes
Copy link
Member

@msujew msujew left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm fine with the changes as they are and I can't see any regressions in the application. After merging this, I'll take care of replacing react-virtualized with react-window.

@vince-fugnitto Any objections?

doc/Migration.md Outdated Show resolved Hide resolved
@AlexandraBuzila
Update React to v18
a49f6d4 
- remove usage of ReactDOM.render and use new root API
- mitigate removal of the React render callback: remove onRender field
from ReactDialog and ReactWidget. Subclasses are now adding the
callbacks themselves.
- list children prop explicitly
- mitigate state batching in SearchInWorkspaceInput
- fix return type in ToolbarImpl.renderGroupsInColumn

Contributed on behalf of STMicroelectronics

Signed-off-by: Alexandra Buzila <abuzila@eclipsesource.com>
@AlexandraBuzila
Update React to v18
34a5487 
Remove npm/npmjs/-/playwright-core/1.22.2 from
dependency-check-baseline.json


Contributed on behalf of STMicroelectronics
Signed-off-by: Alexandra Buzila <abuzila@eclipsesource.com>
@AlexandraBuzila
Update React to v18
35f3b51 
Update migration guide to mention react resolutions from package.json


Contributed on behalf of STMicroelectronics
Signed-off-by: Alexandra Buzila <abuzila@eclipsesource.com>
@AlexandraBuzila
Update React to v18
918b496 
* make containerNodeRoot property readonly in CommentThreadWidget

Contributed on behalf of STMicroelectronics
Signed-off-by: Alexandra Buzila <abuzila@eclipsesource.com>
@AlexandraBuzila
Update React to v18
7238267 
* update version Migration guide

Contributed on behalf of STMicroelectronics
Signed-off-by: Alexandra Buzila <abuzila@eclipsesource.com>
@AlexandraBuzila
Copy link
Contributor Author

I rebased the branch on the lastest master and fixed a conflict.

@msujew
Copy link
Member

msujew commented Aug 9, 2022

@vince-fugnitto Any objections to merging this? I'll be merging this later today.

@vince-fugnitto
Copy link
Member

@msujew I must have missed the ping from #11455 (review), I'm fine with merging and we can do the update we mentioned afterwards.

@msujew msujew merged commit 1626004 into eclipse-theia:master Aug 9, 2022
@msujew msujew mentioned this pull request Aug 10, 2022
Merged
2 tasks
This was referenced Aug 24, 2022
Closed
Closed
@vince-fugnitto vince-fugnitto added this to the 1.29.0 milestone Aug 25, 2022
@vince-fugnitto vince-fugnitto mentioned this pull request Sep 15, 2022
Closed
This was referenced Nov 16, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msujew msujew msujew approved these changes

@vince-fugnitto vince-fugnitto Awaiting requested review from vince-fugnitto

Assignees
No one assigned
Labels
dependencies pull requests that update a dependency file react issues related to the react language
Projects
None yet
Milestone
1.29.0
Development

Successfully merging this pull request may close these issues.
3 participants
@AlexandraBuzila @msujew @vince-fugnitto