You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "EnxDev (via GitHub)" <gi...@apache.org> on 2024/05/07 14:50:31 UTC
[PR] refactor: Migration of Chart to TypeScript [superset]
EnxDev opened a new pull request, #28370:
URL: https://github.com/apache/superset/pull/28370
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Migrate Chart to TypeScript
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N.A.
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
- All tests should pass
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593660897
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1594764817
##########
superset-frontend/src/components/Chart/ChartErrorMessage.tsx:
##########
@@ -21,10 +21,16 @@ import React 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 interface Props {
chartId: string;
error?: SupersetError;
+ subtitle: JSX.Element;
+ copyText: string;
+ link: string | undefined;
Review Comment:
It's equivalent to
```suggestion
link?: string;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593797983
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
Unless we want to actually fix it and assign a timestamp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1594864047
##########
superset-frontend/src/components/Chart/ChartErrorMessage.tsx:
##########
@@ -21,10 +21,16 @@ import React 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 interface Props {
chartId: string;
error?: SupersetError;
+ subtitle: JSX.Element;
+ copyText: string;
+ link: string | undefined;
Review Comment:
There's gotta be a lint rule for that we can add if there's a preference here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593655829
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
-class Chart extends React.PureComponent {
- constructor(props) {
+ renderContainerStartTime: number;
Review Comment:
Is `renderContainerStartTime` actually used anywhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593645443
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -40,69 +41,71 @@ 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,
- labelColors: PropTypes.object,
- sharedLabelColors: 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 interface ChartProps {
+ annotationData?: object;
+ actions: any;
+ chartId: string;
+ datasource?: {
+ database?: {
+ name: string;
+ };
+ };
+ dashboardId?: number;
+ initialValues?: object;
+ formData: QueryFormData;
+ labelColors?: object;
+ sharedLabelColors?: object;
+ width: number;
+ height: number;
+ setControlValue: Function;
+ timeout?: number;
+ vizType: string;
+ triggerRender?: boolean;
+ force?: boolean;
+ isFiltersInitialized?: boolean;
+ chartAlert?: string;
+ chartStatus?: string;
+ chartStackTrace?: string;
+ queriesResponse?: queryResponse[];
+ triggerQuery?: boolean;
+ chartIsStale?: boolean;
+ errorMessage?: React.ReactNode;
+ addFilter?: Function;
+ onQuery?: MouseEventHandler<HTMLSpanElement>;
+ onFilterMenuOpen?: Function;
+ onFilterMenuClose?: Function;
+ ownState?: any;
+ postTransformProps?: Function;
+ datasetsStatus?: 'loading' | 'error' | 'complete';
+ isInView?: boolean;
+ emitCrossFilters?: boolean;
+ renderStartTime?: Date;
+}
+export type queryResponse = {
+ errors: SupersetError[];
+ message: string;
+ link: string;
+};
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 }>`
Review Comment:
We don't use `width` prop in `Styles` so maybe let's just remove it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1681716047
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -214,7 +209,7 @@ class Chart extends React.PureComponent<ChartProps, {}> {
});
}
- renderErrorMessage(queryResponse: queryResponse) {
+ renderErrorMessage(QueryResponse: QueryResponse) {
Review Comment:
renderErrorMessage(queryResponse: QueryResponse)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593662500
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
-class Chart extends React.PureComponent {
- constructor(props) {
+ renderContainerStartTime: number;
+
+ static propTypes: any;
Review Comment:
I don't think that's necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593794793
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1595603018
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
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](https://github.com/apache/superset/commit/5c02e3199b924cc8a45622e660096d6fdc8c5857), 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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593738212
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
I removed this from ChartProps.
You mean we should remove him from line 198 and 200 ( start_offset and duration keys)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593694308
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
-class Chart extends React.PureComponent {
- constructor(props) {
+ renderContainerStartTime: number;
Review Comment:
I removed it, thank you
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
-class Chart extends React.PureComponent {
- constructor(props) {
+ renderContainerStartTime: number;
+
+ static propTypes: any;
Review Comment:
I removed it, thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1595093375
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -40,69 +41,71 @@ 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,
- labelColors: PropTypes.object,
- sharedLabelColors: 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 interface ChartProps {
+ annotationData?: object;
+ actions: any;
Review Comment:
For the annotation `property`, I believe the interface I created in this [PR](https://github.com/apache/superset/pull/28346) should be suitable.
For the `actions` property I created a type.
What do you think about that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1688649592
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
We can prob remove it and add it back in later if we find that we need it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593796701
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -214,7 +209,7 @@ class Chart extends React.PureComponent<ChartProps, {}> {
});
}
- renderErrorMessage(queryResponse: queryResponse) {
+ renderErrorMessage(QueryResponse: QueryResponse) {
Review Comment:
The argument should be lowercase, only the type is PascalCase
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593642398
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -40,69 +41,71 @@ 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,
- labelColors: PropTypes.object,
- sharedLabelColors: 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 interface ChartProps {
+ annotationData?: object;
+ actions: any;
Review Comment:
Can we use more specific types? In general we try to avoid using very generic types like `any` or `object`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1595093375
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -40,69 +41,71 @@ 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,
- labelColors: PropTypes.object,
- sharedLabelColors: 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 interface ChartProps {
+ annotationData?: object;
+ actions: any;
Review Comment:
For the annotation `property`, I believe the interface I created in this [PR](https://github.com/apache/superset/pull/28346) should be suitable.
For the `actions` property I created a type.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593642674
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -40,69 +41,71 @@ 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,
- labelColors: PropTypes.object,
- sharedLabelColors: 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 interface ChartProps {
+ annotationData?: object;
+ actions: any;
+ chartId: string;
+ datasource?: {
+ database?: {
+ name: string;
+ };
+ };
+ dashboardId?: number;
+ initialValues?: object;
+ formData: QueryFormData;
+ labelColors?: object;
+ sharedLabelColors?: object;
+ width: number;
+ height: number;
+ setControlValue: Function;
+ timeout?: number;
+ vizType: string;
+ triggerRender?: boolean;
+ force?: boolean;
+ isFiltersInitialized?: boolean;
+ chartAlert?: string;
+ chartStatus?: string;
+ chartStackTrace?: string;
+ queriesResponse?: queryResponse[];
+ triggerQuery?: boolean;
+ chartIsStale?: boolean;
+ errorMessage?: React.ReactNode;
+ addFilter?: Function;
+ onQuery?: MouseEventHandler<HTMLSpanElement>;
+ onFilterMenuOpen?: Function;
+ onFilterMenuClose?: Function;
+ ownState?: any;
+ postTransformProps?: Function;
+ datasetsStatus?: 'loading' | 'error' | 'complete';
+ isInView?: boolean;
+ emitCrossFilters?: boolean;
+ renderStartTime?: Date;
+}
+export type queryResponse = {
Review Comment:
```suggestion
export type QueryResponse = {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593693631
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -40,69 +41,71 @@ 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,
- labelColors: PropTypes.object,
- sharedLabelColors: 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 interface ChartProps {
+ annotationData?: object;
+ actions: any;
+ chartId: string;
+ datasource?: {
+ database?: {
+ name: string;
+ };
+ };
+ dashboardId?: number;
+ initialValues?: object;
+ formData: QueryFormData;
+ labelColors?: object;
+ sharedLabelColors?: object;
+ width: number;
+ height: number;
+ setControlValue: Function;
+ timeout?: number;
+ vizType: string;
+ triggerRender?: boolean;
+ force?: boolean;
+ isFiltersInitialized?: boolean;
+ chartAlert?: string;
+ chartStatus?: string;
+ chartStackTrace?: string;
+ queriesResponse?: queryResponse[];
+ triggerQuery?: boolean;
+ chartIsStale?: boolean;
+ errorMessage?: React.ReactNode;
+ addFilter?: Function;
+ onQuery?: MouseEventHandler<HTMLSpanElement>;
+ onFilterMenuOpen?: Function;
+ onFilterMenuClose?: Function;
+ ownState?: any;
+ postTransformProps?: Function;
+ datasetsStatus?: 'loading' | 'error' | 'complete';
+ isInView?: boolean;
+ emitCrossFilters?: boolean;
+ renderStartTime?: Date;
+}
+export type queryResponse = {
+ errors: SupersetError[];
+ message: string;
+ link: string;
+};
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 }>`
Review Comment:
I removed it, thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1593935525
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
Yes, I think we should understand why it was put there.
I remain waiting for instructions, thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1603264200
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -36,73 +38,102 @@ 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 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,
- labelColors: PropTypes.object,
- sharedLabelColors: 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 interface ChartProps {
+ annotationData?: Object;
+ actions: Actions;
+ chartId: string;
+ datasource?: {
+ database?: {
+ name: string;
+ };
+ };
+ dashboardId?: number;
+ initialValues?: object;
+ formData: QueryFormData;
+ labelColors?: object;
+ sharedLabelColors?: object;
+ width: number;
+ height: number;
+ setControlValue: Function;
+ timeout?: number;
+ vizType: string;
+ triggerRender?: boolean;
+ force?: boolean;
+ isFiltersInitialized?: boolean;
+ chartAlert?: string;
+ chartStatus?: string;
+ chartStackTrace?: string;
+ queriesResponse?: QueryResponse[];
+ triggerQuery?: boolean;
+ chartIsStale?: boolean;
+ errorMessage?: React.ReactNode;
+ addFilter?: Function;
+ onQuery?: MouseEventHandler<HTMLSpanElement>;
+ onFilterMenuOpen?: Function;
+ onFilterMenuClose?: Function;
+ ownState?: any;
Review Comment:
Can we add a better type for this?
##########
superset-frontend/src/components/Chart/ChartErrorMessage.tsx:
##########
@@ -21,10 +21,16 @@ import React 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 interface Props {
chartId: string;
error?: SupersetError;
+ subtitle: JSX.Element;
+ copyText: string;
+ link?: string;
+ source: ChartSource;
+ stackTrace?: string;
Review Comment:
I believe this could be an extension of `superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts` `ClientErrorObject`
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -36,73 +38,102 @@ 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 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,
- labelColors: PropTypes.object,
- sharedLabelColors: 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 interface ChartProps {
+ annotationData?: Object;
+ actions: Actions;
+ chartId: string;
+ datasource?: {
+ database?: {
+ name: string;
+ };
+ };
+ dashboardId?: number;
+ initialValues?: object;
+ formData: QueryFormData;
+ labelColors?: object;
+ sharedLabelColors?: object;
+ width: number;
+ height: number;
+ setControlValue: Function;
+ timeout?: number;
+ vizType: string;
+ triggerRender?: boolean;
+ force?: boolean;
+ isFiltersInitialized?: boolean;
+ chartAlert?: string;
+ chartStatus?: string;
+ chartStackTrace?: string;
+ queriesResponse?: QueryResponse[];
+ triggerQuery?: boolean;
+ chartIsStale?: boolean;
+ errorMessage?: React.ReactNode;
+ addFilter?: Function;
+ onQuery?: MouseEventHandler<HTMLSpanElement>;
+ onFilterMenuOpen?: Function;
+ onFilterMenuClose?: Function;
+ ownState?: any;
+ postTransformProps?: Function;
+ datasetsStatus?: 'loading' | 'error' | 'complete';
+ isInView?: boolean;
+ emitCrossFilters?: boolean;
+}
+
+export type QueryResponse = {
Review Comment:
This assumes that the response will always be an error. I think we already have a correct type for this. We either need to adjust that to account for the error or create one specifically for when get an error:
Instead of this QueryResponse see:` superset-frontend/packages/superset-ui-core/src/query/types/QueryResponse.ts` `ChartDataResponse`
For the error maybe `superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts` `ClientErrorObject`
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -201,7 +238,7 @@ class Chart extends React.PureComponent {
});
}
- renderErrorMessage(queryResponse) {
+ renderErrorMessage(QueryResponse: QueryResponse) {
Review Comment:
```suggestion
renderErrorMessage(queryResponse: QueryResponse) {
```
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -214,7 +209,7 @@ class Chart extends React.PureComponent<ChartProps, {}> {
});
}
- renderErrorMessage(queryResponse: queryResponse) {
+ renderErrorMessage(QueryResponse: QueryResponse) {
Review Comment:
Does not look like this was resolved
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
Let's proceed with the migration and if we need to fix that we can do it in a separate PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Migration of Chart to TypeScript [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1603286938
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
No `any` please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org