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:29 UTC
(superset) branch chore/fc-07-reactify created (now c5b4ab58222)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a change to branch chore/fc-07-reactify
in repository https://gitbox.apache.org/repos/asf/superset.git
at c5b4ab58222 chore(lint): convert reactify.tsx to function component
This branch includes the following new commits:
new c5b4ab58222 chore(lint): convert reactify.tsx to function component
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: chore(lint): convert reactify.tsx to function component
Posted by ru...@apache.org.
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);
- }));
});