Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

refactor: introduce react-query on api resource hook #21240

Merged

Conversation

justinpark
Copy link
Member

SUMMARY

#13218 introduced a hook for api resources. The main purpose of the hook is for a state management for async api request and transformed a return data to a desired format. As @ktmud suggested, react-query includes these specs and also provides the global cache storage.

This commit introduces react-query to design a new api resource for tables query first.
With the react-query hook, sqlLab can skip the duplicate request for table list on switching each tab. (And it can save the request table query in dataset configuration modal)

After this commit merged, we can replace existing charts and database hooks by react-query too.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

-Before:

before--react-query.mov
  • After:
after--react-query.mov

TESTING INSTRUCTIONS

unit tests

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

cc: @suddjian

@ktmud
Copy link
Member

ktmud commented Aug 29, 2022

I'm always a fan of using good open-source solution sooner than later. :)

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #21240 (0a021db) into master (034ee1c) will decrease coverage by 0.00%.
The diff coverage is 69.44%.

@@            Coverage Diff             @@
##           master   #21240      +/-   ##
==========================================
- Coverage   66.43%   66.43%   -0.01%     
==========================================
  Files        1784     1786       +2     
  Lines       68185    68190       +5     
  Branches     7265     7270       +5     
==========================================
+ Hits        45298    45300       +2     
  Misses      21018    21018              
- Partials     1869     1872       +3     
Flag Coverage Δ
javascript 52.49% <69.44%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (ø)
superset-frontend/src/views/App.tsx 0.00% <0.00%> (ø)
...et-frontend/src/components/TableSelector/index.tsx 75.67% <69.23%> (-2.59%) ⬇️
superset-frontend/src/hooks/apiResources/tables.ts 81.25% <81.25%> (ø)
superset-frontend/src/views/QueryProvider.tsx 100.00% <100.00%> (ø)
superset-frontend/src/SqlLab/actions/sqlLab.js 61.05% <0.00%> (-0.33%) ⬇️
...d/src/SqlLab/components/SaveDatasetModal/index.tsx 52.87% <0.00%> (ø)
...-frontend/src/components/FilterableTable/index.tsx 72.63% <0.00%> (+0.29%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@geido geido 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!

@justinpark justinpark force-pushed the refactor--table-selector-using-react-query branch from 2cb1007 to 506548d Compare August 31, 2022 02:02
@ktmud ktmud merged commit 65a11b6 into apache:master Sep 1, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 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/XL 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants