You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/04/17 17:35:30 UTC
(superset) 01/01: chore(lint): convert reactify.tsx to function component
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch chore/fc-07-reactify
in repository https://gitbox.apache.org/repos/asf/superset.git
commit c5b4ab58222ca50ae3c83b183c3acfe1e2d496c4
Author: Evan Rusackas <ev...@rusackas.com>
AuthorDate: Fri Apr 17 10:35:16 2026 -0700
chore(lint): convert reactify.tsx to function component
Converts the reactify HOC in superset-ui-core from a class-based
wrapper to a function component. Updates the associated unit test.
⚠️ reactify wraps legacy D3/NVD3 render functions — class→function
conversion changes componentDidMount/Update to useEffect, which has
different timing semantics. This PR isolates that change for focused
review.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
.../src/chart/components/reactify.tsx | 119 ++++++++++++---------
.../test/chart/components/reactify.test.tsx | 84 ++++++---------
2 files changed, 104 insertions(+), 99 deletions(-)
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..1fcb13920aa 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,13 @@
* 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 {
+ 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 +54,82 @@ export interface RenderFuncType<Props> {
propTypes?: WeakValidationMap<Props & ReactifyProps>;
}
+export interface ReactifiedComponentRef {
+ container?: HTMLDivElement;
+}
+
+type ReactifiedComponent<Props> = ForwardRefExoticComponent<
+ PropsWithoutRef<Props & ReactifyProps> & RefAttributes<ReactifiedComponentRef>
+> & {
+ defaultProps?: Partial<Props & ReactifyProps>;
+ propTypes?: WeakValidationMap<Props & ReactifyProps>;
+};
+
export default function reactify<Props extends object>(
renderFn: RenderFuncType<Props>,
callbacks?: LifeCycleCallbacks,
-): ComponentClass<Props & ReactifyProps> {
- class ReactifiedComponent extends Component<Props & ReactifyProps> {
- container?: HTMLDivElement;
-
- constructor(props: Props & ReactifyProps) {
- super(props);
- this.setContainerRef = this.setContainerRef.bind(this);
- }
-
- componentDidMount() {
- this.execute();
- }
-
- componentDidUpdate() {
- this.execute();
- }
-
- componentWillUnmount() {
- this.container = undefined;
- if (callbacks?.componentWillUnmount) {
- callbacks.componentWillUnmount.bind(this)();
- }
- }
-
- setContainerRef(ref: HTMLDivElement) {
- this.container = ref;
- }
-
- execute() {
- if (this.container) {
- renderFn(this.container, this.props);
+): ReactifiedComponent<Props> {
+ const ReactifiedComponent = forwardRef<
+ ReactifiedComponentRef,
+ Props & ReactifyProps
+ >(function ReactifiedComponent(props, ref) {
+ const containerRef = useRef<HTMLDivElement>(null);
+
+ // Expose container via ref for external access
+ useImperativeHandle(
+ ref,
+ () => ({
+ get container() {
+ return containerRef.current ?? undefined;
+ },
+ }),
+ [],
+ );
+
+ // Execute renderFn on mount and every update (mimics componentDidMount + componentDidUpdate)
+ useEffect(() => {
+ if (containerRef.current) {
+ renderFn(containerRef.current, props);
}
- }
+ });
+
+ // Cleanup on unmount
+ useEffect(
+ () => () => {
+ if (callbacks?.componentWillUnmount) {
+ // Preserve legacy behavior where `this` was a component instance
+ // exposing a `container` property
+ callbacks.componentWillUnmount.call({
+ container: containerRef.current,
+ });
+ }
+ },
+ [],
+ );
+
+ const { id, className } = props;
+
+ return <div ref={containerRef} id={id} className={className} />;
+ });
- render() {
- const { id, className } = this.props;
-
- return <div ref={this.setContainerRef} id={id} className={className} />;
- }
+ if (renderFn.displayName) {
+ ReactifiedComponent.displayName = renderFn.displayName;
}
- const ReactifiedClass: ComponentClass<Props & ReactifyProps> =
- ReactifiedComponent;
+ // Cast to any to assign propTypes and defaultProps since forwardRef
+ // components have complex typing that makes direct assignment difficult
+ const result = ReactifiedComponent as any;
- if (renderFn.displayName) {
- ReactifiedClass.displayName = renderFn.displayName;
- }
- // eslint-disable-next-line react/forbid-foreign-prop-types
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 ReactifiedComponent<Props>;
}
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..fddbf59289d 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,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');
- 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);
- }));
+ 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', async () => {
+ 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);
- }));
});