address review: widen reactify return type to ComponentType

Preserves backward compatibility for TypeScript consumers that
previously saw ComponentClass<Props & ReactifyProps>. The new
ReactifiedComponent<Props> type is now exported so callers that want
the ref-aware surface can narrow to it explicitly.

superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx

@@ -17,8 +17,14 @@
  * under the License.
  */
 
-// eslint-disable-next-line no-restricted-syntax -- whole React import is required for `reactify.test.tsx` Jest test passing.
-import { Component, ComponentClass, WeakValidationMap } from 'react';
+import { forwardRef, useEffect, useImperativeHandle, useRef } from 'react';
+import type {
+  ComponentType,
+  WeakValidationMap,
+  ForwardRefExoticComponent,
+  PropsWithoutRef,
+  RefAttributes,
+} from 'react';
 
 // TODO: Note that id and className can collide between Props and ReactifyProps
 // leading to (likely) unexpected behaviors. We should either require Props to not
@@ -49,66 +55,90 @@ export interface RenderFuncType<Props> {
   propTypes?: WeakValidationMap<Props & ReactifyProps>;
 }
 
+export interface ReactifiedComponentRef {
+  container?: HTMLDivElement;
+}
+
+export type ReactifiedComponent<Props> = ForwardRefExoticComponent<
+  PropsWithoutRef<Props & ReactifyProps> & RefAttributes<ReactifiedComponentRef>
+>;
+
+// Return the widest public type that covers "use it as a React component" so
+// TypeScript consumers who previously relied on `ComponentClass<...>` still
+// compile. Callers that want the new ref-aware surface can narrow to
+// `ReactifiedComponent<Props>` explicitly.
 export default function reactify<Props extends object>(
   renderFn: RenderFuncType<Props>,
   callbacks?: LifeCycleCallbacks,
-): ComponentClass<Props & ReactifyProps> {
-  class ReactifiedComponent extends Component<Props & ReactifyProps> {
-    container?: HTMLDivElement;
+): ComponentType<Props & ReactifyProps> {
+  const ReactifiedComponent = forwardRef<
+    ReactifiedComponentRef,
+    Props & ReactifyProps
+  >(function ReactifiedComponent(props, ref) {
+    const containerRef = useRef<HTMLDivElement>(null);
+    // Keep the latest props available to the unmount callback — legacy
+    // consumers read values off `this.props` (e.g. ReactNVD3 uses id).
+    const propsRef = useRef(props);
+    propsRef.current = props;
 
-    constructor(props: Props & ReactifyProps) {
-      super(props);
-      this.setContainerRef = this.setContainerRef.bind(this);
-    }
+    // Expose container via ref for external access
+    useImperativeHandle(
+      ref,
+      () => ({
+        get container() {
+          return containerRef.current ?? undefined;
+        },
+      }),
+      [],
+    );
 
-    componentDidMount() {
-      this.execute();
-    }
-
-    componentDidUpdate() {
-      this.execute();
-    }
-
-    componentWillUnmount() {
-      this.container = undefined;
-      if (callbacks?.componentWillUnmount) {
-        callbacks.componentWillUnmount.bind(this)();
+    // Execute renderFn on mount and every update (mimics componentDidMount + componentDidUpdate)
+    useEffect(() => {
+      if (containerRef.current) {
+        renderFn(containerRef.current, props);
       }
-    }
+    });
 
-    setContainerRef(ref: HTMLDivElement) {
-      this.container = ref;
-    }
+    // Cleanup on unmount
+    useEffect(
+      () => () => {
+        if (callbacks?.componentWillUnmount) {
+          // Preserve legacy behavior where `this` was a component instance
+          // exposing `props`. The class version cleared `this.container`
+          // before invoking componentWillUnmount, so mirror that here to
+          // prevent callbacks from touching a DOM node that's being torn
+          // down.
+          callbacks.componentWillUnmount.call({
+            container: undefined,
+            props: propsRef.current,
+          });
+        }
+      },
+      [],
+    );
 
-    execute() {
-      if (this.container) {
-        renderFn(this.container, this.props);
-      }
-    }
+    const { id, className } = props;
 
-    render() {
-      const { id, className } = this.props;
-
-      return <div ref={this.setContainerRef} id={id} className={className} />;
-    }
-  }
-
-  const ReactifiedClass: ComponentClass<Props & ReactifyProps> =
-    ReactifiedComponent;
+    return <div ref={containerRef} id={id} className={className} />;
+  });
 
   if (renderFn.displayName) {
-    ReactifiedClass.displayName = renderFn.displayName;
+    ReactifiedComponent.displayName = renderFn.displayName;
   }
-  // eslint-disable-next-line react/forbid-foreign-prop-types
+
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any -- forwardRef static field types don't line up with renderFn's validator types
+  const result = ReactifiedComponent as any;
+
   if (renderFn.propTypes) {
-    ReactifiedClass.propTypes = {
-      ...ReactifiedClass.propTypes,
+    result.propTypes = {
+      ...result.propTypes,
       ...renderFn.propTypes,
     };
   }
+
   if (renderFn.defaultProps) {
-    ReactifiedClass.defaultProps = renderFn.defaultProps;
+    result.defaultProps = renderFn.defaultProps;
   }
 
-  return ReactifiedComponent;
+  return result as unknown as ComponentType<Props & ReactifyProps>;
 }

superset-frontend/packages/superset-ui-core/test/chart/components/reactify.test.tsx

@@ -19,9 +19,9 @@
 
 import '@testing-library/jest-dom';
 import PropTypes from 'prop-types';
-import { PureComponent } from 'react';
+import { useEffect, useState } from 'react';
 import { reactify } from '@superset-ui/core';
-import { render, screen } from '@testing-library/react';
+import { render, screen, waitFor } from '@testing-library/react';
 import { RenderFuncType } from '../../../src/chart/components/reactify';
 
 describe('reactify(renderFn)', () => {
@@ -52,48 +52,36 @@ describe('reactify(renderFn)', () => {
     componentWillUnmount: willUnmountCb,
   });
 
-  class TestComponent extends PureComponent<{}, { content: string }> {
-    constructor(props = {}) {
-      super(props);
-      this.state = { content: 'abc' };
-    }
+  function TestComponent() {
+    const [content, setContent] = useState('abc');
 
-    componentDidMount() {
-      setTimeout(() => {
-        this.setState({ content: 'def' });
+    useEffect(() => {
+      const timer = setTimeout(() => {
+        setContent('def');
       }, 10);
-    }
+      return () => clearTimeout(timer);
+    }, []);
 
-    render() {
-      const { content } = this.state;
-
-      return <TheChart id="test" content={content} />;
-    }
+    return <TheChart id="test" content={content} />;
   }
 
-  class AnotherTestComponent extends PureComponent<{}, {}> {
-    render() {
-      return <TheChartWithWillUnmountHook id="another_test" />;
-    }
+  function AnotherTestComponent() {
+    return <TheChartWithWillUnmountHook id="another_test" />;
   }
 
-  test('returns a React component class', () =>
-    new Promise(done => {
-      render(<TestComponent />);
+  test('returns a React component and re-renders on prop changes', async () => {
+    render(<TestComponent />);
 
-      expect(renderFn).toHaveBeenCalledTimes(1);
-      expect(screen.getByText('abc')).toBeInTheDocument();
-      expect(screen.getByText('abc').parentNode).toHaveAttribute('id', 'test');
-      setTimeout(() => {
-        expect(renderFn).toHaveBeenCalledTimes(2);
-        expect(screen.getByText('def')).toBeInTheDocument();
-        expect(screen.getByText('def').parentNode).toHaveAttribute(
-          'id',
-          'test',
-        );
-        done(undefined);
-      }, 20);
-    }));
+    expect(renderFn).toHaveBeenCalledTimes(1);
+    expect(screen.getByText('abc')).toBeInTheDocument();
+    expect(screen.getByText('abc').parentNode).toHaveAttribute('id', 'test');
+
+    await waitFor(() => {
+      expect(screen.getByText('def')).toBeInTheDocument();
+    });
+    expect(screen.getByText('def').parentNode).toHaveAttribute('id', 'test');
+    expect(renderFn).toHaveBeenCalledTimes(2);
+  });
   describe('displayName', () => {
     test('has displayName if renderFn.displayName is defined', () => {
       expect(TheChart.displayName).toEqual('BoldText');
@@ -126,20 +114,16 @@ describe('reactify(renderFn)', () => {
       expect(AnotherChart.defaultProps).toBeUndefined();
     });
   });
-  test('does not try to render if not mounted', () => {
+  test('calls renderFn when container is set', () => {
     const anotherRenderFn = jest.fn();
-    const AnotherChart = reactify(anotherRenderFn); // enables valid new AnotherChart() call
-    // @ts-expect-error
-    new AnotherChart({ id: 'test' }).execute();
-    expect(anotherRenderFn).not.toHaveBeenCalled();
+    const AnotherChart = reactify(anotherRenderFn);
+    const { unmount } = render(<AnotherChart id="test" />);
+    expect(anotherRenderFn).toHaveBeenCalled();
+    unmount();
+  });
+  test('calls willUnmount hook when it is provided', () => {
+    const { unmount } = render(<AnotherTestComponent />);
+    unmount();
+    expect(willUnmountCb).toHaveBeenCalledTimes(1);
   });
-  test('calls willUnmount hook when it is provided', () =>
-    new Promise(done => {
-      const { unmount } = render(<AnotherTestComponent />);
-      setTimeout(() => {
-        unmount();
-        expect(willUnmountCb).toHaveBeenCalledTimes(1);
-        done(undefined);
-      }, 20);
-    }));
 });