Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Migration of Chart to TypeScript #28370

Merged
merged 9 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,18 @@
* specific language governing permissions and limitations
* under the License.
*/
import PropTypes from 'prop-types';
import { PureComponent } from 'react';
import {
ensureIsArray,
FeatureFlag,
isFeatureEnabled,
logging,
QueryFormData,
styled,
t,
SqlaFormData,
ClientErrorObject,
ChartDataResponse,
} from '@superset-ui/core';
import { PLACEHOLDER_DATASOURCE } from 'src/dashboard/constants';
import Loading from 'src/components/Loading';
Expand All @@ -36,73 +39,98 @@ import { getUrlParam } from 'src/utils/urlUtils';
import { isCurrentUserBot } from 'src/utils/isBot';
import { ChartSource } from 'src/types/ChartSource';
import { ResourceStatus } from 'src/hooks/apiResources/apiResources';
import { Dispatch } from 'redux';
import { Annotation } from 'src/explore/components/controls/AnnotationLayerControl';
import ChartRenderer from './ChartRenderer';
import { ChartErrorMessage } from './ChartErrorMessage';
import { getChartRequiredFieldsMissingMessage } from '../../utils/getChartRequiredFieldsMissingMessage';

const propTypes = {
annotationData: PropTypes.object,
actions: PropTypes.object,
chartId: PropTypes.number.isRequired,
datasource: PropTypes.object,
// current chart is included by dashboard
dashboardId: PropTypes.number,
// original selected values for FilterBox viz
// so that FilterBox can pre-populate selected values
// only affect UI control
initialValues: PropTypes.object,
// formData contains chart's own filter parameter
// and merged with extra filter that current dashboard applying
formData: PropTypes.object.isRequired,
labelsColor: PropTypes.object,
labelsColorMap: PropTypes.object,
width: PropTypes.number,
height: PropTypes.number,
setControlValue: PropTypes.func,
timeout: PropTypes.number,
vizType: PropTypes.string.isRequired,
triggerRender: PropTypes.bool,
force: PropTypes.bool,
isFiltersInitialized: PropTypes.bool,
// state
chartAlert: PropTypes.string,
chartStatus: PropTypes.string,
chartStackTrace: PropTypes.string,
queriesResponse: PropTypes.arrayOf(PropTypes.object),
triggerQuery: PropTypes.bool,
chartIsStale: PropTypes.bool,
errorMessage: PropTypes.node,
// dashboard callbacks
addFilter: PropTypes.func,
onQuery: PropTypes.func,
onFilterMenuOpen: PropTypes.func,
onFilterMenuClose: PropTypes.func,
ownState: PropTypes.object,
postTransformProps: PropTypes.func,
datasetsStatus: PropTypes.oneOf(['loading', 'error', 'complete']),
isInView: PropTypes.bool,
emitCrossFilters: PropTypes.bool,
};
export type ChartErrorType = Partial<ClientErrorObject>;
export interface ChartProps {
annotationData?: Annotation;
actions: Actions;
chartId: string;
datasource?: {
database?: {
name: string;
};
};
dashboardId?: number;
initialValues?: object;
formData: QueryFormData;
labelColors?: string;
sharedLabelColors?: string;
width: number;
height: number;
setControlValue: Function;
timeout?: number;
vizType: string;
triggerRender?: boolean;
force?: boolean;
isFiltersInitialized?: boolean;
chartAlert?: string;
chartStatus?: string;
chartStackTrace?: string;
queriesResponse: ChartDataResponse[];
triggerQuery?: boolean;
chartIsStale?: boolean;
errorMessage?: React.ReactNode;
addFilter?: (type: string) => void;
onQuery?: () => void;
onFilterMenuOpen?: (chartId: string, column: string) => void;
onFilterMenuClose?: (chartId: string, column: string) => void;
ownState: boolean;
postTransformProps?: Function;
datasetsStatus?: 'loading' | 'error' | 'complete';
isInView?: boolean;
emitCrossFilters?: boolean;
}

export type Actions = {
logEvent(
LOG_ACTIONS_RENDER_CHART: string,
arg1: {
slice_id: string;
has_err: boolean;
error_details: string;
start_offset: number;
ts: number;
duration: number;
},
): Dispatch;
chartRenderingFailed(
arg0: string,
chartId: string,
arg2: string | null,
): Dispatch;
postChartFormData(
formData: SqlaFormData,
arg1: boolean,
timeout: number | undefined,
chartId: string,
dashboardId: number | undefined,
ownState: boolean,
): Dispatch;
};
const BLANK = {};
const NONEXISTENT_DATASET = t(
'The dataset associated with this chart no longer exists',
);

const defaultProps = {
const defaultProps: Partial<ChartProps> = {
addFilter: () => BLANK,
onFilterMenuOpen: () => BLANK,
onFilterMenuClose: () => BLANK,
initialValues: BLANK,
setControlValue() {},
setControlValue: () => BLANK,
triggerRender: false,
dashboardId: null,
chartStackTrace: null,
dashboardId: undefined,
chartStackTrace: undefined,
force: false,
isInView: true,
};

const Styles = styled.div`
const Styles = styled.div<{ height: number; width?: number }>`
EnxDev marked this conversation as resolved.
Show resolved Hide resolved
min-height: ${p => p.height}px;
position: relative;
text-align: center;
Expand Down Expand Up @@ -150,9 +178,12 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
class Chart extends PureComponent<ChartProps, {}> {
static defaultProps = defaultProps;

class Chart extends PureComponent {
constructor(props) {
renderStartTime: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does renderStartTime come from? In the old version, we're only reading it, but we don't assign any value to it. In your changes you put renderStartTime in ChartProps, but also as class's field. I don't think it's actually passed as a prop anywhere, and as a class field it's also only read, not assigned.
So maybe we should just remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this from ChartProps.
You mean we should remove him from line 198 and 200 ( start_offset and duration keys)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, unless I misunderstand something here, renderStartTime is always undefined because we don’t set its value anywhere, so maybe we could remove it from the code. @rusackas any objections?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we want to actually fix it and assign a timestamp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should understand why it was put there.
I remain waiting for instructions, thank you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if it's not wired up, that's an odd problem. It sure seems like it'd be useful for logging, which seems to be its intent. There's one in ChartRenderer.jsx that IS wired up, for reference. Maybe we don't need both though?

It WAS set in the original commit, actually. I haven't checked to see when its initialization was removed but clearly there wasn't much outcry.

I'd err on the side of more logging, not less. Maybe @eschutho would be interested in taking more advantage of this insight :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can prob remove it and add it back in later if we find that we need it.


constructor(props: ChartProps) {
super(props);
this.handleRenderContainerFailure =
this.handleRenderContainerFailure.bind(this);
Expand Down Expand Up @@ -182,7 +213,10 @@ class Chart extends PureComponent {
);
}

handleRenderContainerFailure(error, info) {
handleRenderContainerFailure(
error: Error,
info: { componentStack: string } | null,
) {
const { actions, chartId } = this.props;
logging.warn(error);
actions.chartRenderingFailed(
Expand All @@ -201,7 +235,7 @@ class Chart extends PureComponent {
});
}

renderErrorMessage(queryResponse) {
renderErrorMessage(queryResponse: ChartErrorType) {
const {
chartId,
chartAlert,
Expand Down Expand Up @@ -241,14 +275,14 @@ class Chart extends PureComponent {
error={error}
subtitle={<MonospaceDiv>{message}</MonospaceDiv>}
copyText={message}
link={queryResponse ? queryResponse.link : null}
link={queryResponse ? queryResponse.link : undefined}
source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
stackTrace={chartStackTrace}
/>
);
}

renderSpinner(databaseName) {
renderSpinner(databaseName: string | undefined) {
const message = databaseName
? t('Waiting on %s', databaseName)
: t('Waiting on database...');
Expand Down Expand Up @@ -290,12 +324,15 @@ class Chart extends PureComponent {
queriesResponse = [],
width,
} = this.props;

const databaseName = datasource?.database?.name;

const isLoading = chartStatus === 'loading';
this.renderContainerStartTime = Logger.getTimestamp();

if (chartStatus === 'failed') {
return queriesResponse.map(item => this.renderErrorMessage(item));
return queriesResponse.map(item =>
this.renderErrorMessage(item as ChartErrorType),
);
}

if (errorMessage && ensureIsArray(queriesResponse).length === 0) {
Expand All @@ -307,7 +344,6 @@ class Chart extends PureComponent {
/>
);
}

if (
!isLoading &&
!chartAlert &&
Expand Down Expand Up @@ -354,8 +390,4 @@ class Chart extends PureComponent {
);
}
}

Chart.propTypes = propTypes;
Chart.defaultProps = defaultProps;

export default Chart;
12 changes: 9 additions & 3 deletions superset-frontend/src/components/Chart/ChartErrorMessage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,21 @@
* under the License.
*/

import { ClientErrorObject, SupersetError } from '@superset-ui/core';
import { FC } from 'react';
import { SupersetError } from '@superset-ui/core';
import { useChartOwnerNames } from 'src/hooks/apiResources';
import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
import { ChartSource } from 'src/types/ChartSource';

interface Props {
export type Props = {
chartId: string;
error?: SupersetError;
}
subtitle: JSX.Element;
copyText: string | undefined;
link?: string;
source: ChartSource;
stackTrace?: string;
} & Omit<ClientErrorObject, 'error'>;

/**
* fetches the chart owners and adds them to the extra data of the error message
Expand Down
Loading