diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx index c52f13d0de0..80552d4ee8b 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx @@ -17,8 +17,20 @@ * 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, + useLayoutEffect, + 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 +61,103 @@ export interface RenderFuncType { propTypes?: WeakValidationMap; } +export interface ReactifiedComponentRef { + container?: HTMLDivElement; +} + +export type ReactifiedComponent = ForwardRefExoticComponent< + PropsWithoutRef & RefAttributes +>; + +// Return the widest public type that covers "use it as a React component" so +// TypeScript JSX callers and `ComponentType<...>`-typed variables still compile; +// callers with explicit `ComponentClass<...>` annotations must widen to +// `ComponentType`. Those wanting the forwardRef surface can narrow to +// `ReactifiedComponent` explicitly. export default function reactify( renderFn: RenderFuncType, callbacks?: LifeCycleCallbacks, -): ComponentClass { - class ReactifiedComponent extends Component { - container?: HTMLDivElement; +): ComponentType { + const ReactifiedComponent = forwardRef< + ReactifiedComponentRef, + Props & ReactifyProps + >(function ReactifiedComponent(props, ref) { + const containerRef = useRef(null); + // Keep the latest props available to the unmount callback — legacy + // consumers read values off `this.props` (e.g. ReactNVD3 uses id). + // Update the ref in a layout effect rather than during render so the + // assignment only happens for committed renders (safe under Concurrent + // Mode) and is in place before the passive unmount effect reads it. + const propsRef = useRef(props); + useLayoutEffect(() => { + 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) { + // `forwardRef` widens the props parameter to `PropsWithoutRef<...>`, + // which TypeScript can't narrow back to `Props & ReactifyProps` when + // `Props` is a generic `object`. The values are identical at runtime, + // so assert the original prop shape for `renderFn`. + renderFn( + containerRef.current, + props as Readonly, + ); } - } + }); - 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
; - } - } - - const ReactifiedClass: ComponentClass = - ReactifiedComponent; + return
; + }); 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; } diff --git a/superset-frontend/packages/superset-ui-core/test/chart/components/reactify.test.tsx b/superset-frontend/packages/superset-ui-core/test/chart/components/reactify.test.tsx index b64e1989411..f06eb34c742 100644 --- a/superset-frontend/packages/superset-ui-core/test/chart/components/reactify.test.tsx +++ b/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,41 @@ 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 ; - } + return ; } - class AnotherTestComponent extends PureComponent<{}, {}> { - render() { - return ; - } + function AnotherTestComponent() { + return ; } - test('returns a React component class', () => - new Promise(done => { - render(); + beforeEach(() => { + (renderFn as jest.Mock).mockClear(); + willUnmountCb.mockClear(); + }); - 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); - })); + test('returns a React component and re-renders on prop changes', async () => { + render(); + + 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 +119,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(); + expect(anotherRenderFn).toHaveBeenCalled(); + unmount(); + }); + test('calls willUnmount hook when it is provided', () => { + const { unmount } = render(); + unmount(); + expect(willUnmountCb).toHaveBeenCalledTimes(1); }); - test('calls willUnmount hook when it is provided', () => - new Promise(done => { - const { unmount } = render(); - setTimeout(() => { - unmount(); - expect(willUnmountCb).toHaveBeenCalledTimes(1); - done(undefined); - }, 20); - })); });