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

chore: migrate QueryTable component from jsx to tsx #17944

Conversation

MayUWish
Copy link
Contributor

@MayUWish MayUWish commented Jan 6, 2022

SUMMARY

  • updated Query type at superset-frontend/src/SqlLab/types.ts/Query and extended it into QueryTable
  • updated defaultQueryLimit to be optional at ResultSetProps of superset-frontend/src/SqlLab/components/ResultSet/index.tsx
  • moved statusAttributes into useMemo hook so that it will not make the dependencies of useMemo Hook change on every render and statusAttributes is only used within useMomo hook
  • QueryTable is used in the following places
    image

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@MayUWish MayUWish changed the title Meitong qu/query table type script conversion chore: migrate QueryTable component from jsx to tsx Jan 6, 2022
@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #17944 (881cd1c) into master (765c72a) will decrease coverage by 1.02%.
The diff coverage is 100.00%.

❗ Current head 881cd1c differs from pull request most recent head 7cc7c73. Consider uploading reports for the commit 7cc7c73 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17944      +/-   ##
==========================================
- Coverage   67.08%   66.05%   -1.03%     
==========================================
  Files        1611     1591      -20     
  Lines       64919    62416    -2503     
  Branches     6871     6288     -583     
==========================================
- Hits        43548    41227    -2321     
- Misses      19504    19567      +63     
+ Partials     1867     1622     -245     
Flag Coverage Δ
javascript 50.86% <100.00%> (-2.93%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rt-controls/src/operators/rollingWindowOperator.ts 100.00% <ø> (ø)
...ui-chart-controls/src/operators/utils/constants.ts 100.00% <ø> (ø)
...-chart-controls/src/sections/advancedAnalytics.tsx 33.33% <ø> (ø)
.../superset-ui-core/src/connection/SupersetClient.ts 100.00% <ø> (ø)
...superset-ui-core/src/query/types/PostProcessing.ts 100.00% <ø> (ø)
.../src/time-format/TimeFormatterRegistrySingleton.ts 100.00% <ø> (ø)
...ackages/superset-ui-core/src/utils/featureFlags.ts 100.00% <ø> (ø)
...s/legacy-plugin-chart-country-map/src/countries.ts 100.00% <ø> (ø)
...legacy-plugin-chart-partition/src/controlPanel.tsx 25.00% <ø> (ø)
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx 50.00% <ø> (ø)
... and 651 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 765c72a...7cc7c73. Read the comment docs.

"plugins:create-patch-version": "npm run prune && ./scripts/lernaVersion.sh patch",
"plugins:create-conventional-version": "npm run prune && lerna version --conventional-commits --create-release github --yes",
"plugins:create-minor-version": "npm run prune && lerna version minor --yes",
"plugins:create-patch-version": "npm run prune && lerna version patch --yes",
Copy link
Member

Choose a reason for hiding this comment

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

did you mean to change this?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you need to rebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May I know how I can do the rebase? I noticed that package.json and package.lock.json of my files were changed(probably due to installation), thus I copied these 2 files from master branch of this repo and pasted on mine, then I did this pull request. I just compared with master branch one more time, it has line 54-56 in green. Shall I just replace line 54-56 in green with the original line 56 in red?

Copy link
Member

Choose a reason for hiding this comment

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

do you have a remote tracking apache?

Copy link
Member

Choose a reason for hiding this comment

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

if so you can go to your master branch, do a git pull apache master, then go back to this branch and do a git rebase and walk through it. It is a little confusing and can be scary so I am happy to walk you through it if you want.

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, plz. Can I have your help to walk me through it? Thank you

Copy link
Member

@AAfghahi AAfghahi left a comment

Choose a reason for hiding this comment

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

Really impressive job! I left some comments, and there seems to be something happening with your package.json that is messing things up. I think a rebase would help, happy to walk you through that.

"plugins:create-patch-version": "npm run prune && ./scripts/lernaVersion.sh patch",
"plugins:create-conventional-version": "npm run prune && lerna version --conventional-commits --create-release github --yes",
"plugins:create-minor-version": "npm run prune && lerna version minor --yes",
"plugins:create-patch-version": "npm run prune && lerna version patch --yes",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you need to rebase?

onDbClicked: () => {},
};
// query's type is original Query; Shallow-copy of query, q's type is QueryTableQuery. So that prop, sql passed to another component will remain string, the type of original Query
interface QueryTableQueryTemp1 extends Omit<Query, 'sql'> {
Copy link
Member

Choose a reason for hiding this comment

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

ohh I didn't know that you could do Omit!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Without Omit, I cannot 'override' it :))

Copy link
Member

Choose a reason for hiding this comment

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

I thought you could with extend, but I guess since it is an already existing field then that would be weird

);

const user = useSelector(({ sqlLab: { user } }) => user);
const user = useSelector((state: any) => state.sqlLab.user);
Copy link
Member

Choose a reason for hiding this comment

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

Can we get more specific than state: any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it on line 38, 39, 94


const data = useMemo(() => {
const restoreSql = query => {
props.actions.queryEditorSetSql({ id: query.sqlEditorId }, query.sql);
const restoreSql = (query: Record<string, any>) => {
Copy link
Member

Choose a reason for hiding this comment

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

can you use the Query object that you imported here instead of Record<string, any>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated on line 97

actions?.queryEditorSetSql({ id: query.sqlEditorId }, query.sql);
};

const openQueryInNewTab = (query: Record<string, any>) => {
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member

Choose a reason for hiding this comment

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

There are a couple of them, same thought all the way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated on line 101, 105, 109, 113. Thank you!

@@ -13138,7 +13138,7 @@ msgid ""
msgstr ""

#: superset/templates/superset/fab_overrides/list_with_checkboxes.html:82
msgid "Use the edit buttom to change this field"
msgid "Use the edit button to change this field"
Copy link
Member

Choose a reason for hiding this comment

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

I think you can rebase and this should go away

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May I know how I can do the rebase to fix it? I though the green one 'button' is correct?(misspelling typo changed by other contributor I guess? not sure) Thank you

@MayUWish MayUWish force-pushed the MeitongQu/QueryTableTypeScriptConversion branch from 2468fa0 to 2a37fc4 Compare January 7, 2022 20:51
@MayUWish MayUWish force-pushed the MeitongQu/QueryTableTypeScriptConversion branch from 2a37fc4 to 9a2cef4 Compare January 7, 2022 20:54
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

I have a question about the custom interface, in general we really don't want to have mixed types and it's not clear why we would need it here.

Comment on lines 42 to 48
interface QueryTableQueryTemp1 extends Omit<Query, 'sql'> {
sql: string | Record<string, any>;
}

interface QueryTableQueryTemp2 extends Omit<QueryTableQueryTemp1, 'progress'> {
progress: number | Record<string, any>;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
interface QueryTableQueryTemp1 extends Omit<Query, 'sql'> {
sql: string | Record<string, any>;
}
interface QueryTableQueryTemp2 extends Omit<QueryTableQueryTemp1, 'progress'> {
progress: number | Record<string, any>;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! combination of types are eliminated.

Comment on lines 50 to 52
interface QueryTableQuery extends Omit<QueryTableQueryTemp2, 'state'> {
state: string | Record<string, any>;
}
Copy link
Member

Choose a reason for hiding this comment

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

You can Omit several keys:

Suggested change
interface QueryTableQuery extends Omit<QueryTableQueryTemp2, 'state'> {
state: string | Record<string, any>;
}
interface QueryTableQuery extends Omit<Query, 'state' | 'sql' | 'progress'> {
state: string | Record<string, any>;
sql: string | Record<string, any>;
progress: number | Record<string, any>;
}

But in general it's not a good idea to have attributes with multiple types like this. We've had a lot of errors in the past caused by storing attributes with different types, and it leads to complicated logic that needs to check types (like you did below where you check if state is a string).

Why do you need to store these attributes as records?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi ~ Thank you for the suggestion about omitting several keys all together. And for the question about union types string | Record<string, any> (or number | Record<string, any>), they(state, sql, progress) were string/number from Query initially, but then at useMemo function, they were assigned to be an element/component ( such as q.state, q.progress, q.state). Do you have any suggestion in such cases? Thank you very much!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated and combination of types are eliminated.

);

const user = useSelector(({ sqlLab: { user } }) => user);
const user = useSelector((state: RootState) => state.sqlLab.user);
Copy link
Member

Choose a reason for hiding this comment

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

can we instead use the generics for useSelector, which are the RootState useSelector<RootState, User>and then the type for the return value, in this case, user. Also for the RootState, do you mind create a type file for SqlLab like we have for dashboards? https://github.com/apache/superset/blob/master/superset-frontend/src/dashboard/types.ts#L96

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I just updated the PR for other comments, which did not include update for this comment. I expect to spend more time working on this. (plus learning more about redux vs typescript) Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type for sqlLab redux is defined at 'superset-frontend/src/SqlLab/types.ts', please let me know if there is any adjustment is needed. Especially for databases and tables, there are very general types , object. If more details are needed, Please let me know where I can refer to for their types? Thank you very much.

image

Copy link
Member

Choose a reason for hiding this comment

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

That looks good. The databases and tables will be intentionally left vague because we do not know the structure of the user's data.
If you were to write this line with generics as const user = useSelector<RootState, User>(state) => state.sqlLab.user); does it work ok with the type you have there for RootState?

Copy link
Contributor Author

@MayUWish MayUWish Jan 25, 2022

Choose a reason for hiding this comment

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

Replaced it with 'const user = useSelector<RootState, User>(state => state.sqlLab.user)' and 'import { UserWithPermissionsAndRoles as User } from 'src/types/bootstrapTypes';'

@MayUWish MayUWish force-pushed the MeitongQu/QueryTableTypeScriptConversion branch from e0a1cd4 to c0ae220 Compare January 20, 2022 02:13
@AAfghahi
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@AAfghahi Ephemeral environment spinning up at http://54.70.159.149:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@AAfghahi
Copy link
Member

@yousoph and @rosemarie-chiu Meitong added screenshots for qa testing

@MayUWish MayUWish force-pushed the MeitongQu/QueryTableTypeScriptConversion branch 2 times, most recently from ef9a9af to ae6d7c3 Compare January 25, 2022 02:14
@MayUWish MayUWish force-pushed the MeitongQu/QueryTableTypeScriptConversion branch from ae6d7c3 to a54d755 Compare January 27, 2022 20:25
Copy link
Member

@lyndsiWilliams lyndsiWilliams left a comment

Choose a reason for hiding this comment

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

Just a couple of very tiny nits, otherwise this looks great! Awesome work on this 😁

superset-frontend/src/SqlLab/types.ts Outdated Show resolved Hide resolved
noDuplicate: boolean;
};

/** Root state of redux */
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, I think the code explains well enough without needing a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed. Thank you

@MayUWish MayUWish force-pushed the MeitongQu/QueryTableTypeScriptConversion branch from a54d755 to e75b4df Compare January 28, 2022 20:34
@MayUWish MayUWish force-pushed the MeitongQu/QueryTableTypeScriptConversion branch from e75b4df to 7cc7c73 Compare January 28, 2022 20:40
Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you @MayUWish!

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Thank you!

@lyndsiWilliams lyndsiWilliams merged commit 9f678e5 into apache:master Feb 11, 2022
@lyndsiWilliams lyndsiWilliams deleted the MeitongQu/QueryTableTypeScriptConversion branch February 11, 2022 12:54
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L Superset-Community-Partners Preset community partner program participants 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants