Upgrade to React 18 #1100

jernestmyers · 2024-05-20T16:52:09Z

No description provided.

dmfalke · 2024-06-27T17:46:26Z

Regarding ReactDOM.unmountComponentAtNode, this goes hand-in-hand with ReactDOM.render. Per https://react.dev/blog/2022/03/08/react-18-upgrade-guide#updates-to-client-rendering-apis, you should be able to replace those instances with root.unmount(). This might involve adding an instance property to class based components, in order to keep track of root.

dmfalke · 2024-06-27T17:58:05Z

As noted elsewhere, ReactDOM.findDOMNode is deprecated. In most cases, this can be replaced with a ref. This can be a follow-up task. Just noting here for posterity.

❯ rg findDOMNode -l
packages/libs/wdk-client/src/Components/InputControls/IndeterminateCheckbox.tsx
packages/libs/wdk-client/src/Components/Loading/Loading.tsx
packages/libs/wdk-client/src/Components/Overlays/Popup.tsx
packages/libs/wdk-client/src/Components/Display/Sticky.jsx
packages/libs/wdk-client/src/Controllers/LegacyParamController.tsx
packages/libs/wdk-client/src/Components/AttributeFilter/FieldList.jsx
packages/libs/wdk-client/src/Components/AttributeFilter/Histogram.jsx
packages/libs/wdk-client/src/Views/Answer/AnswerFilter.jsx
packages/libs/wdk-client/src/Views/Records/RecordUI.jsx

dmfalke

I've reviewed the code changes, and it looks really great. I made some nitpicky suggestions, mostly related to keeping things as type-safe as possible. Let me know what you think!

dmfalke · 2024-06-27T18:12:35Z

packages/libs/components/src/plots/facetedPlots/FacetedPiePlot.tsx

@@ -26,7 +26,7 @@ type FacetedPiePlotProps = Omit<

 const FacetedPiePlot = (facetedPiePlotProps: FacetedPiePlotProps) => {
  const { componentProps } = facetedPiePlotProps;
-  const onPlotlyRender = useCallback((_, graphDiv) => {
+  const onPlotlyRender = useCallback((_: any, graphDiv: HTMLElement) => {


Let's use unknown instead of any. It's a safer type to use, especially in this case where we don't want do anything with the parameter.

dmfalke · 2024-06-27T18:43:16Z

packages/libs/components/src/stories/widgets/DateInput.stories.tsx

@@ -14,7 +14,7 @@ export default {
 const ControlledTemplate: Story<DateInputProps> = (args) => {
  const [value, setValue] = useState<string>('2005-01-03');
  const onValueChange = useCallback(
-    (newValue) => {
+    (newValue: any) => {


Should the type of newValue be string | undefined?

dmfalke · 2024-06-27T18:48:52Z

packages/libs/components/src/stories/widgets/DateRangeInput.stories.tsx

@@ -21,15 +21,15 @@ export const ControlledLinked: Story<DateRangeInputProps> = () => {
  // there must be a cleverer way to do this
  // avoiding the cut and paste
  const handleChangeA = useCallback(
-    (newRange) => {
+    (newRange: any) => {


Ideally, this would be DateRange, but the expected type is DateOrNumberRange. I suppose we can switch this to DateOrNumberRange and then check that it is a DataRange and throw an error (or return early) if not. But let's just leave it as any for now.

Ultimately, we should fix DateRange to use the correct types, but that's beyond the scope of this PR.

dmfalke · 2024-06-27T18:50:01Z

packages/libs/components/src/stories/widgets/NumberInput.stories.tsx

@@ -14,7 +14,7 @@ export default {
 const ControlledTemplate: Story<NumberInputProps> = (args) => {
  const [value, setValue] = useState<number | undefined>(args.value ?? 1);
  const onValueChange = useCallback(
-    (newValue) => {
+    (newValue: any) => {


Similar comment as above, about DateRange.

dmfalke · 2024-06-27T18:56:32Z

packages/libs/eda/src/lib/core/components/filter/HistogramFilter.tsx

@@ -337,7 +337,7 @@ export function HistogramFilter(props: Props) {
      className="filter-param"
      style={{ position: 'relative', marginTop: '2em' }}
    >
-      {data.error && <pre>{String(data.error)}</pre>}
+      <>{data.error && <pre>{String(data.error)}</pre>}</>


This could be changed to:

Suggested change

<>{data.error && <pre>{String(data.error)}</pre>}</>

{data.error != null && <pre>{String(data.error)}</pre>}

dmfalke · 2024-06-27T19:05:10Z

packages/libs/eda/src/lib/workspace/Variable.tsx

+            <>
+              <div className={cx('-ProviderLabelPrefix')}>
+                <i>
+                  Original variable{' '}
+                  {MultiFilterVariable.is(variable) ? 'names' : 'name'}:
+                </i>
+              </div>
+              {/* showing three variables for multifilter or single variable */}
+              &nbsp;{threeProviderLabel}
+              {/* generalize Show/Hide more: there is a case that providerLabel is string */}
+              {Array.isArray(providerLabel) && providerLabel.length > 3 ? (
+                <>
+                  {showMore && providerLabelLeftover}
+                  &nbsp;
+                  <HelpIcon>
+                    The name of this variable in the original data files
+                  </HelpIcon>
+                  &nbsp;&nbsp;
+                  <button
+                    className="variable-show-more-link link"
+                    onClick={() => {
+                      setShowMore(!showMore);
+                    }}
+                  >
+                    {showMoreLink}
+                  </button>
+                </>
+              ) : (
+                <>
+                  &nbsp;
+                  <HelpIcon>
+                    The name of this variable in the original data files
+                  </HelpIcon>
+                </>
+              )}
+            </>


We can isolate the use of the fragment to the text node.

Suggested change

<>

<div className={cx('-ProviderLabelPrefix')}>

<i>

Original variable{' '}

{MultiFilterVariable.is(variable) ? 'names' : 'name'}:

</i>

</div>

{/* showing three variables for multifilter or single variable */}

 {threeProviderLabel}

{/* generalize Show/Hide more: there is a case that providerLabel is string */}

{Array.isArray(providerLabel) && providerLabel.length > 3 ? (

<>

{showMore && providerLabelLeftover}

 

<HelpIcon>

The name of this variable in the original data files

</HelpIcon>

  

<button

className="variable-show-more-link link"

onClick={() => {

setShowMore(!showMore);

}}

>

{showMoreLink}

</button>

</>

) : (

<>

 

<HelpIcon>

The name of this variable in the original data files

</HelpIcon>

</>

)}

</>

<div className={cx('-ProviderLabelPrefix')}>

<i>

Original variable{' '}

{MultiFilterVariable.is(variable) ? 'names' : 'name'}:

</i>

</div>

{/* showing three variables for multifilter or single variable */}

<> {threeProviderLabel}</>

{/* generalize Show/Hide more: there is a case that providerLabel is string */}

{Array.isArray(providerLabel) && providerLabel.length > 3 ? (

<>

{showMore && providerLabelLeftover}

 

<HelpIcon>

The name of this variable in the original data files

</HelpIcon>

  

<button

className="variable-show-more-link link"

onClick={() => {

setShowMore(!showMore);

}}

>

{showMoreLink}

</button>

</>

) : (

<>

 

<HelpIcon>

The name of this variable in the original data files

</HelpIcon>

</>

)}

dmfalke · 2024-06-27T19:10:32Z

packages/libs/wdk-client/src/Components/Display/CollapsibleSection.tsx

@@ -15,7 +15,7 @@ interface Props {
  isCollapsed?: boolean;
  onCollapsedChange: (isCollapsed: boolean) => void;
  headerContent: React.ReactNode;
-  headerComponent?: React.ReactType;
+  headerComponent?: React.FC | React.ComponentType;


ComponentType is a union of ComponentClass and FunctionComponent.

Suggested change

headerComponent?: React.FC | React.ComponentType;

headerComponent?: React.ComponentType;

dmfalke · 2024-06-27T19:21:14Z

packages/libs/web-common/src/controllers/LegacyMapRedirectHandler.tsx

@@ -81,7 +89,7 @@ export function LegacyMapRedirectHandler({
        const additionalAnalysisConfig = {
          ...baseAdditionalAnalysisConfig,
          ...descriptorConfig,
-        };
+        } as AdditionalAnalysisConfig;


Instead of using as, we should declare the type of the variable:

const additionalAnalysisConfig: AdditionalAnalysisConfig = {

This will require declare the components that make up this value (baseAdditionalAnalysisConfig, and descriptorConfig). I think this will reveal a subtle type conflict.

Generally, we should avoid as if possible, and document why we are using it if we need it. This isn't something we've really enforced before.

dmfalke · 2024-06-27T19:23:38Z

...ges/sites/genomics-site/webapp/wdkCustomization/js/client/components/Downloads/Downloads.tsx

@@ -26,6 +26,8 @@ export function Downloads() {
    [history, location, match]
  );

+  console.log('version:', React.version);


Can we remove this?

dmfalke · 2024-06-27T19:25:01Z

...ite/webapp/wdkCustomization/js/client/components/genomeSummaryView/EmptyChromosomeFilter.tsx

-    chromosomes
-  </div>
-);
+export const EmptyChromosomesFilter: React.FunctionComponent<EmptyChromosomesFilterProps> =


Should we use React.FC to be consistent with other packages?

jernestmyers added 15 commits May 9, 2024 12:12

Upgrade react version

7c30f46

Update root-level react resolution

733a5fc

Resolve React upgrade issues in coreui

b949c20

Resolve issues w/ react v18 upgrade in components library

231954c

Resolve react upgrade errors in wdk-client

f4e20ff

Resolve react 18 upgrade issues in eda

8b5d061

Upgrade emotion packages

7777f19

Resolve react 18 upgrade errors in web-common

f9ce89b

Resolve some key, defaultProps and children errors with react 18 upgrade

775cdca

Replace ReactDOM.render methods

09814f4

Merge branch 'main' into upgrade-react

8bc06e7

Resolve TS errors in genomics-site w/ React 18 upgrade

18fb0da

Merge branch 'main' into upgrade-react

d1b897c

Resolve getPageSection typescript errors

aadfac6

Upgrade notistack to latest version

bccf0fc

dmfalke requested changes Jun 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to React 18 #1100

Upgrade to React 18 #1100

jernestmyers commented May 20, 2024

dmfalke commented Jun 27, 2024

dmfalke commented Jun 27, 2024 •

edited

Loading

dmfalke left a comment

dmfalke Jun 27, 2024

dmfalke Jun 27, 2024

dmfalke Jun 27, 2024

dmfalke Jun 27, 2024

dmfalke Jun 27, 2024

dmfalke Jun 27, 2024

dmfalke Jun 27, 2024

dmfalke Jun 27, 2024

dmfalke Jun 27, 2024

dmfalke Jun 27, 2024

	<>{data.error && <pre>{String(data.error)}</pre>}</>
	{data.error != null && <pre>{String(data.error)}</pre>}

	headerComponent?: React.FC \| React.ComponentType;
	headerComponent?: React.ComponentType;

Upgrade to React 18 #1100

Are you sure you want to change the base?

Upgrade to React 18 #1100

Conversation

jernestmyers commented May 20, 2024

dmfalke commented Jun 27, 2024

dmfalke commented Jun 27, 2024 • edited Loading

dmfalke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmfalke commented Jun 27, 2024 •

edited

Loading