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

React 18 #324

Merged
merged 35 commits into from
Jan 7, 2025
Merged

React 18 #324

merged 35 commits into from
Jan 7, 2025

Conversation

toloudis
Copy link
Contributor

@toloudis toloudis commented Sep 24, 2024

Estimated time to review: 20-30 min?
Most of the changes are converting "class components" to "functional components"

Coauthored with @frasercl
Resolves much of #191 (React lifecycle takes up extra cpu cycles during playback)

  • Upgrade to React 18, bump some other dependency versions.

  • Convert to functional components everywhere.

  • Actually use production mode for production builds.

  • Some updates to SharedCheckbox

future TODOs clean up some seemingly harmless console warnings:
1.

Warning: validateDOMNesting(...): <p> cannot appear as a descendant of <p>. Error Component Stack
    at p (<anonymous>)
    at p (<anonymous>)
    at li (<anonymous>)
    at O (styled-components.br…ser.esm.js:32:23549)
    at ul (<anonymous>)
    at O (styled-components.br…ser.esm.js:32:23549)
    at div (<anonymous>)
    at O (styled-components.br…ser.esm.js:32:23549)
    at div (<anonymous>)
    at LandingPage (index.tsx:235:33)
  1. (antd Tooltip uses findDOMNode)
Warning: findDOMNode is deprecated and will be removed in the next major release. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://reactjs.org/link/strict-mode-find-node Error Component Stack
    at SingleObserver (index.js:23:24)
    at ResizeObserver (index.js:24:24)
    at eval (index.js:61:34)
    at Tooltip (Tooltip.js:23:32)
    at eval (index.js:52:16)
    at div (<anonymous>)
    at div (<anonymous>)
    at div (<anonymous>)
    at Toolbar (index.tsx:61:24)
  1. (nouislider-react is out of date and still using defaultProps)
Warning: v: Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead. Error Component Stack
    at v (nouislider-react.umd…ction.min.js:1:3395)
    at SmarterSlider (SmarterSlider.tsx:13:35)
    at span (<anonymous>)
    at span (<anonymous>)
    at SliderRow (index.tsx:34:3)
    at PlaySliderRow (index.tsx:123:54)

Base automatically changed from feature/override-defaults to main October 15, 2024 16:04
Copy link

github-actions bot commented Oct 25, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2025-01-07 20:09 UTC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

file got autoformatted: the only changes in gh actions workflows are to use node 20 instead of 18

@@ -2,7 +2,7 @@

%axis-override {
/*slider settings*/
.noUi-handle {
.noUi-horizontal .noUi-handle {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes a bug with how slider handles are rendered

mode: "development",
devtool: "eval-source-map",
mode: env.env === "production" ? "production" : "development",
devtool: env.env === "production" ? "source-map" : "eval-source-map",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

key change: production mode actually uses production mode. TODO: see how easy it is to debug using source-map in production while still getting higher performance of production mode.

},
maxEntrypointSize: 3512000,
maxAssetSize: 3512000,
},
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 block cleans up warnings/errors in devServer when testing production mode

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the max asset size 3.5 MB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we ever exceed it we can just increase these amounts or decide on splitting.. this is just a starting point that makes things work

@toloudis toloudis marked this pull request as ready for review October 30, 2024 00:07
@toloudis toloudis requested a review from a team as a code owner October 30, 2024 00:07
@toloudis toloudis requested review from rugeli and ascibisz and removed request for a team October 30, 2024 00:07
ShrimpCryptid added a commit that referenced this pull request Nov 4, 2024
…nator

- Timepoint denominator now matches maximum slider value.
- Sorted imports.
- Copied a change from #324 which fixes a rendering bug in the slider.
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid left a comment

Choose a reason for hiding this comment

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

Always fun to watch React class components get turned into functional ones!

@toloudis toloudis merged commit 4180a09 into main Jan 7, 2025
4 checks passed
@toloudis toloudis deleted the feature/react18perf-functionize branch January 7, 2025 20:03
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.

4 participants