You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/07/15 04:14:40 UTC

[GitHub] [incubator-superset] etr2460 opened a new pull request #10317: chore: type welcome

etr2460 opened a new pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317


   ### SUMMARY
   As titled. Also moves some of the types from `profile` into a shared `bootstrapTypes` file. I suspect these will go away once bootstrap templates are gone, in favor of well defined response types from functions that call APIs
   
   ### TEST PLAN
   CI and manual testing. The tests didn't seem super great here, so I assume they're more ensuring that things render, so i made sure to test interactions within the ui as well.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   to: @ktmud @kristw @williaster 


----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] ktmud commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r455402056



##########
File path: superset-frontend/src/welcome/DashboardTable.tsx
##########
@@ -17,35 +17,45 @@
  * under the License.
  */
 import React from 'react';
-import PropTypes from 'prop-types';
 import { t } from '@superset-ui/translation';
 import { SupersetClient } from '@superset-ui/connection';
 import moment from 'moment';
 import { debounce } from 'lodash';
 import ListView from 'src/components/ListView/ListView';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { Dashboard } from 'src/types/bootstrapTypes';
+import { FetchDataConfig } from 'src/components/ListView/types';
 
 const PAGE_SIZE = 25;
 
-class DashboardTable extends React.PureComponent {
-  static propTypes = {
-    addDangerToast: PropTypes.func.isRequired,
-    search: PropTypes.string,
-  };
+interface DashboardTableProps {
+  addDangerToast: (message: string) => void;
+  search?: string;
+}
+
+interface DashboardTableState {
+  dashboards: Dashboard[];
+  dashboard_count: number;
+  loading: boolean;
+}
 
+class DashboardTable extends React.PureComponent<
+  DashboardTableProps,
+  DashboardTableState
+> {
   state = {
     dashboards: [],
     dashboard_count: 0,
     loading: false,
   };
 
-  componentDidUpdate(prevProps) {
+  componentDidUpdate(prevProps: DashboardTableProps) {
     if (prevProps.search !== this.props.search) {
       this.fetchDataDebounced({
         pageSize: PAGE_SIZE,
         pageIndex: 0,
         sortBy: this.initialSort,
-        filters: {},
+        filters: [],

Review comment:
       Why are we changing this to array?




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] etr2460 merged pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
etr2460 merged pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317


   


----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] kristw commented on pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
kristw commented on pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#issuecomment-658539991


   Is the PR title pun intended?


----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] kristw commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r454786169



##########
File path: superset-frontend/src/welcome/DashboardTable.tsx
##########
@@ -17,35 +17,45 @@
  * under the License.
  */
 import React from 'react';
-import PropTypes from 'prop-types';
 import { t } from '@superset-ui/translation';
 import { SupersetClient } from '@superset-ui/connection';
 import moment from 'moment';
 import { debounce } from 'lodash';
 import ListView from 'src/components/ListView/ListView';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { Dashboard } from 'src/types/bootstrapTypes';
+import { FetchDataConfig } from 'src/components/ListView/types';
 
 const PAGE_SIZE = 25;
 
-class DashboardTable extends React.PureComponent {
-  static propTypes = {
-    addDangerToast: PropTypes.func.isRequired,
-    search: PropTypes.string,
-  };
+interface DashboardTableProps {

Review comment:
       airbnb guide use `type` for `Props` instead of `interface`.

##########
File path: superset-frontend/src/types/bootstrapTypes.ts
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+export type User = {
+  createdOn: string;
+  email: string;
+  firstName: string;
+  isActive: boolean;
+  lastName: string;
+  userId: number;
+  username: string;
+};
+
+export interface UserWithPermissionsAndRoles extends User {
+  permissions: {
+    database_access?: string[];
+    datasource_access?: string[];
+  };
+  roles: Record<string, any>;

Review comment:
       can it be more specific than `any`?




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] nytai commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r455158598



##########
File path: superset-frontend/src/welcome/App.tsx
##########
@@ -44,7 +44,7 @@ setupApp();
 setupPlugins();
 
 const container = document.getElementById('app');
-const bootstrap = JSON.parse(container.getAttribute('data-bootstrap'));
+const bootstrap = JSON.parse(container?.getAttribute('data-bootstrap') ?? '{}');

Review comment:
       is `??` a new operator? 




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] ktmud commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r455406730



##########
File path: superset-frontend/src/welcome/DashboardTable.tsx
##########
@@ -58,6 +68,13 @@ class DashboardTable extends React.PureComponent {
         row: {
           original: { url, dashboard_title: dashboardTitle },
         },
+      }: {
+        row: {
+          original: {
+            url: string;
+            dashboard_title: string;
+          };
+        };

Review comment:
       Sorry for missing this earlier, but I think all `Cell` renderers actually have the same signature. 
   
   `react-table` has typing for its parameters, so this should probably be refactored as:
   
   ```ts
   import { Column } from 'react-table'
   
   interface DataboardRecord {
     url: string;
     dashboard_title: string;
     changed_on: string;
     changed_by_name: string;
     changedByUrl: string;
   }
   
   columns: Column<DataboardRecord>[] = [{
      ...
      Cell: ({ row: { original: { ... } } }) => ...
   }]
   ```




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] kristw commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r455201101



##########
File path: superset-frontend/src/welcome/DashboardTable.tsx
##########
@@ -17,35 +17,45 @@
  * under the License.
  */
 import React from 'react';
-import PropTypes from 'prop-types';
 import { t } from '@superset-ui/translation';
 import { SupersetClient } from '@superset-ui/connection';
 import moment from 'moment';
 import { debounce } from 'lodash';
 import ListView from 'src/components/ListView/ListView';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { Dashboard } from 'src/types/bootstrapTypes';
+import { FetchDataConfig } from 'src/components/ListView/types';
 
 const PAGE_SIZE = 25;
 
-class DashboardTable extends React.PureComponent {
-  static propTypes = {
-    addDangerToast: PropTypes.func.isRequired,
-    search: PropTypes.string,
-  };
+interface DashboardTableProps {

Review comment:
       The point is not the code won't work. Both works.
   
   Using `type` has convenience when hovering and you can see its content immediately because `type` is just an alias while `interface` encapsulates its content so you have to go to the definition to see its fields.
   
   Oftentimes you want to see what can be in the `Props`.
   
   when using type
   
   ![image](https://user-images.githubusercontent.com/1659771/87573999-94a34e80-c682-11ea-9bd8-821e18d4df19.png)
   
   when using interface
   
   ![image](https://user-images.githubusercontent.com/1659771/87574025-9ff67a00-c682-11ea-97f7-4e97528275b5.png)
   




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r454791493



##########
File path: superset-frontend/src/types/bootstrapTypes.ts
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+export type User = {
+  createdOn: string;
+  email: string;
+  firstName: string;
+  isActive: boolean;
+  lastName: string;
+  userId: number;
+  username: string;
+};
+
+export interface UserWithPermissionsAndRoles extends User {
+  permissions: {
+    database_access?: string[];
+    datasource_access?: string[];
+  };
+  roles: Record<string, any>;

Review comment:
       updated the type




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] ktmud commented on pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#issuecomment-659059289


   @villebro I think this one is pretty safe. Just verified locally that the welcome and profile page still work. Not sure how it looks like on the cherry-picked branch though. 


----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] kristw commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r455201101



##########
File path: superset-frontend/src/welcome/DashboardTable.tsx
##########
@@ -17,35 +17,45 @@
  * under the License.
  */
 import React from 'react';
-import PropTypes from 'prop-types';
 import { t } from '@superset-ui/translation';
 import { SupersetClient } from '@superset-ui/connection';
 import moment from 'moment';
 import { debounce } from 'lodash';
 import ListView from 'src/components/ListView/ListView';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { Dashboard } from 'src/types/bootstrapTypes';
+import { FetchDataConfig } from 'src/components/ListView/types';
 
 const PAGE_SIZE = 25;
 
-class DashboardTable extends React.PureComponent {
-  static propTypes = {
-    addDangerToast: PropTypes.func.isRequired,
-    search: PropTypes.string,
-  };
+interface DashboardTableProps {

Review comment:
       The point is not the code is not working. Both works.
   
   Using `type` has convenience when hovering and you can see its content immediately because `type` is just an alias while `interface` encapsulates its content so you have to go to the definition to see its fields.
   
   Oftentimes you want to see what can be in the `Props`.
   
   when using type
   
   ![image](https://user-images.githubusercontent.com/1659771/87573999-94a34e80-c682-11ea-9bd8-821e18d4df19.png)
   
   when using interface
   
   ![image](https://user-images.githubusercontent.com/1659771/87574025-9ff67a00-c682-11ea-97f7-4e97528275b5.png)
   




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] villebro commented on pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#issuecomment-659003946


   @ktmud @etr2460 do you think this is safe to cherry pick for 0.37? getting a merge conflict for a 0.37 blocker ( #10321 ) that is risky to resolve manually, would prefer to pick this to be able to cleanly merge the other one.


----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] ktmud commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r455273818



##########
File path: superset-frontend/src/welcome/App.tsx
##########
@@ -44,7 +44,7 @@ setupApp();
 setupPlugins();
 
 const container = document.getElementById('app');
-const bootstrap = JSON.parse(container.getAttribute('data-bootstrap'));
+const bootstrap = JSON.parse(container?.getAttribute('data-bootstrap') ?? '{}');

Review comment:
       Yes, it's new since TS v3.7: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#nullish-coalescing




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] kristw commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r455201101



##########
File path: superset-frontend/src/welcome/DashboardTable.tsx
##########
@@ -17,35 +17,45 @@
  * under the License.
  */
 import React from 'react';
-import PropTypes from 'prop-types';
 import { t } from '@superset-ui/translation';
 import { SupersetClient } from '@superset-ui/connection';
 import moment from 'moment';
 import { debounce } from 'lodash';
 import ListView from 'src/components/ListView/ListView';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { Dashboard } from 'src/types/bootstrapTypes';
+import { FetchDataConfig } from 'src/components/ListView/types';
 
 const PAGE_SIZE = 25;
 
-class DashboardTable extends React.PureComponent {
-  static propTypes = {
-    addDangerToast: PropTypes.func.isRequired,
-    search: PropTypes.string,
-  };
+interface DashboardTableProps {

Review comment:
       The point is not the code won't work. Both works.
   
   Using `type` has convenience when hovering and you can see its content immediately because `type` is just an alias while `interface` encapsulates its content so you have to go to the definition to see its fields.
   
   Oftentimes you want to see what can be in the `Props` (that's why the recommendation was only for `Props`).
   
   when using type
   
   ![image](https://user-images.githubusercontent.com/1659771/87573999-94a34e80-c682-11ea-9bd8-821e18d4df19.png)
   
   when using interface
   
   ![image](https://user-images.githubusercontent.com/1659771/87574025-9ff67a00-c682-11ea-97f7-4e97528275b5.png)
   




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] kristw commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r455241171



##########
File path: superset-frontend/src/welcome/DashboardTable.tsx
##########
@@ -17,35 +17,45 @@
  * under the License.
  */
 import React from 'react';
-import PropTypes from 'prop-types';
 import { t } from '@superset-ui/translation';
 import { SupersetClient } from '@superset-ui/connection';
 import moment from 'moment';
 import { debounce } from 'lodash';
 import ListView from 'src/components/ListView/ListView';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { Dashboard } from 'src/types/bootstrapTypes';
+import { FetchDataConfig } from 'src/components/ListView/types';
 
 const PAGE_SIZE = 25;
 
-class DashboardTable extends React.PureComponent {
-  static propTypes = {
-    addDangerToast: PropTypes.func.isRequired,
-    search: PropTypes.string,
-  };
+interface DashboardTableProps {

Review comment:
       Generally, when the call can go either way (both with pros/cons or none have good reasons), I would lean on consistency if it is not expensive change and no clear win. Otherwise the entire codebase will have many different dialects. That is probably worse.
   
   If you think the benefit above is not convincing enough and the majority in this repo is already using `interface`, we can also start documenting these decisions for `incubator-superset`, and adopt `interface` for this repo.
   
   For the internal Airbnb project, I'll stick to the TS style guide from the TS working group.




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r454791436



##########
File path: superset-frontend/src/welcome/DashboardTable.tsx
##########
@@ -17,35 +17,45 @@
  * under the License.
  */
 import React from 'react';
-import PropTypes from 'prop-types';
 import { t } from '@superset-ui/translation';
 import { SupersetClient } from '@superset-ui/connection';
 import moment from 'moment';
 import { debounce } from 'lodash';
 import ListView from 'src/components/ListView/ListView';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { Dashboard } from 'src/types/bootstrapTypes';
+import { FetchDataConfig } from 'src/components/ListView/types';
 
 const PAGE_SIZE = 25;
 
-class DashboardTable extends React.PureComponent {
-  static propTypes = {
-    addDangerToast: PropTypes.func.isRequired,
-    search: PropTypes.string,
-  };
+interface DashboardTableProps {

Review comment:
       oops... Well throughout Superset we've got 44 `interface` Props and 10 `type` Props. I'll go through and clean them all up (and try to find a lint rule) sometime in the future. For now I'd rather stick to the more common choice.




----------------------------------------------------------------
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.

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


[GitHub] [incubator-superset] ktmud commented on a change in pull request #10317: chore: type welcome

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10317:
URL: https://github.com/apache/incubator-superset/pull/10317#discussion_r454804319



##########
File path: superset-frontend/src/welcome/DashboardTable.tsx
##########
@@ -17,35 +17,45 @@
  * under the License.
  */
 import React from 'react';
-import PropTypes from 'prop-types';
 import { t } from '@superset-ui/translation';
 import { SupersetClient } from '@superset-ui/connection';
 import moment from 'moment';
 import { debounce } from 'lodash';
 import ListView from 'src/components/ListView/ListView';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { Dashboard } from 'src/types/bootstrapTypes';
+import { FetchDataConfig } from 'src/components/ListView/types';
 
 const PAGE_SIZE = 25;
 
-class DashboardTable extends React.PureComponent {
-  static propTypes = {
-    addDangerToast: PropTypes.func.isRequired,
-    search: PropTypes.string,
-  };
+interface DashboardTableProps {

Review comment:
       I'm not sure about this recommendation. I like how interfaces gives you an alias and really makes intellisense and error messages more readable. Even TypeScript's official doc uses Interface: https://www.typescriptlang.org/docs/handbook/jsx.html#function-component
   
   We have used interface in a lot of places and I have yet to see any problem.




----------------------------------------------------------------
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.

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