chore: Convert QueryAutoRefresh to TypeScript functional React component [sc-48362] #20179

eric-briscoe · 2022-05-24T23:04:01Z

Convert QueryAutoRefresh to TypeScript functional React component

SUMMARY

This PR converts QueryAutoRefresh from es6 class component to TypeScript functional component as part of bigger effort to migrate frontend code to TypeScript and functional React components defined in: #18388

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

There should be no visual change to the application

TESTING INSTRUCTIONS

Open SQL Lab / SQL Editor
Click the "RUN" button
Ensure Query runs and updates results with same behavior that existed prior to this PR
Test with both default behavior and asynchronous query execution on for a database

Changes to http requests for getting Query statuses:
If you watch the dev tools network tab you should see less network traffic as the execution now ensures only one outstanding http call to check query status is made at a time to avoid http response race conditions that can lead to incorrect application state and to reduce tandem network calls on slower connections. If the call fails, the offline state change will be triggered but the interval timer will continue attempting to reconnect and set user online and show results once the connection is restored. This can be tested by:

Opening the dev tools / Network / set the connection from "No Throttling" to "Offline", 2. Click the "Run" button as described in the initial test steps above
See the network failure and the UI will display an "Offline" pill (this is not new UI behavior)
Set the network back to "No Throttling" and wait a few seconds and the next interval will make a successful query status API call and you will see data results again.

QueryAutoRefresh was separated into two files to separate Redux knowledge from the simple functional component:

QueryAutoRefresh/index.ts - Simple functional component with no knowledge of Redux
QueryAutoRefresh/QueryAutoRefreshContainer.ts - Functional component container with connection to redux state that imports and contains QueryAutoRefresh/index.ts component

IMPORTANT:
When changing QueryAutoRefresh to a functional component the behavior of setInterval no longer worked correctly. This is a known issue with functional React components using setInterval can provide stale state / props in the interval callback creating undesired, hard to debug / fix behavior. A new utility hook (SqlLab/utils/useInterval.ts) was introduced in this PR to handle this based on this very helpful article by Dan Abramov (thank you!): https://overreacted.io/making-setinterval-declarative-with-react-hooks/

ADDITIONAL INFORMATION

Has associated issue: [TRACKER] Migrate JavaScript files to TypeScript #18388
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

…8362]'

superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx

eschutho

Thanks @eric-briscoe, this is great! The comments make it very readable, too.

Removes unneeded props and state tracking of offline, adds finally block to simplify clearing pending request, simplifies value comparison in array by using includes in place of indexOf

…esh-to-functional-component

AAfghahi

Looks good! Does it need an ephemeral for testing?

superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx

superset-frontend/src/SqlLab/fixtures.ts

codecov · 2022-05-25T15:38:51Z

Codecov Report

Merging #20179 (6aa313e) into master (a833674) will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##           master   #20179   +/-   ##
=======================================
  Coverage   66.74%   66.75%           
=======================================
  Files        1739     1740    +1     
  Lines       65130    65135    +5     
  Branches     6898     6898           
=======================================
+ Hits        43472    43480    +8     
+ Misses      19908    19906    -2     
+ Partials     1750     1749    -1

Flag	Coverage Δ
javascript	`51.81% <75.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/SqlLab/types.ts	`57.14% <ø> (ø)`
superset-frontend/src/SqlLab/utils/useInterval.ts	`66.66% <66.66%> (ø)`
...d/src/SqlLab/components/QueryAutoRefresh/index.tsx	`70.83% <70.83%> (ø)`
...packages/superset-ui-core/src/query/types/Query.ts	`100.00% <100.00%> (ø)`
...erset-frontend/src/SqlLab/components/App/index.jsx	`57.89% <100.00%> (+2.33%)`	⬆️
superset-frontend/src/SqlLab/fixtures.ts	`100.00% <100.00%> (ø)`
superset-frontend/src/SqlLab/actions/sqlLab.js	`60.22% <0.00%> (-0.29%)`	⬇️
...d/src/SqlLab/components/SaveDatasetModal/index.tsx	`35.61% <0.00%> (+0.94%)`	⬆️

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 a833674...6aa313e. Read the comment docs.

Original implementation had string literals used in multiple places representing Query.state value options. This commit creates a formal TypeScript enum for QueryState so we can remove string literals and ensure better consistency

…esh-to-functional-component

This commit resolves an issue why the TypeScript typing for queryList was marked as a Query[] but was actually a Dictionary (associative array) or Queries. A new type QueryDictionary has been added and the QueryAutoRefresh code was adjusted to use QueryDictionary instead of Query[] in appropriate places as well as unit tests. Commit also removes QueryAutoRefreshContainer by making the once component using QueryAutoRefresh (which is already redux connected) pass the needed values on props. this simplifies the code base and reduce files that need unit testing while keeping QueryAutoRefresh out of needing a redux connection directly.

…esh-to-functional-component

…List [sc-48362] In previous implementation 'scheduled' was not included int he list of Query States. Further investigation shows it should be added to as a running state.

AAfghahi · 2022-05-25T23:19:52Z

@eric-briscoe do you have the pre-commit hook installed? We have it in a makefile. The command is make pre-commit

There is also a way to make prettier run on save, which I forget how to do, but I think we have documented somewhere. Elizabeth definitely knows.

…wrapper object [sc-48362]

…esh-to-functional-component

hughhhh · 2022-06-06T13:43:29Z

superset-frontend/src/SqlLab/utils/useInterval.ts

+ * The useInterval function solves this issue.
+ * more info: https://overreacted.io/making-setinterval-declarative-with-react-hooks/
+ */
+function useInterval(callback: Function, delay: number | null): void {


nit: can we set delay as an optional and = to 0

function useInterval(callback: Function, delay: number = 0): void {

@hughhhh I'm thinking it may be dangerous to run the function continuously without any delay interval (or the minimum interval that the browser allows). If you forget to pass a delay, this could be problematic. I think @eric-briscoe has a good idea about doing nothing (returning a noop). WDYT?

…esh-to-functional-component

Merges in updates from master and resolves conflicts from relocation of some of the Query TypeScript definitions into core

eschutho

Thanks @eric-briscoe, looks great! There are just some tests and checks that aren't passing still.

…esh-to-functional-component

… message [sc-48362]

…core [sc-48362]

…ent [sc-48362] (apache#20179) * git commit -m 'Convert QueryAutoRefresh to functional component [sc-48362]' * addressing PR comments [sc-48362] Removes unneeded props and state tracking of offline, adds finally block to simplify clearing pending request, simplifies value comparison in array by using includes in place of indexOf * Address PR comment to use enum for QueryState [sc-48362] Original implementation had string literals used in multiple places representing Query.state value options. This commit creates a formal TypeScript enum for QueryState so we can remove string literals and ensure better consistency * Address PR comments for object type validation [sc-48362] This commit resolves an issue why the TypeScript typing for queryList was marked as a Query[] but was actually a Dictionary (associative array) or Queries. A new type QueryDictionary has been added and the QueryAutoRefresh code was adjusted to use QueryDictionary instead of Query[] in appropriate places as well as unit tests. Commit also removes QueryAutoRefreshContainer by making the once component using QueryAutoRefresh (which is already redux connected) pass the needed values on props. this simplifies the code base and reduce files that need unit testing while keeping QueryAutoRefresh out of needing a redux connection directly. * Addresses PR comment to add QueryState.SCHEDULED to runningQueryStateList [sc-48362] In previous implementation 'scheduled' was not included int he list of Query States. Further investigation shows it should be added to as a running state. * Fix prettier lint error [sc-48362] * Adjust unit tests for props update hoisting callbacks out of actions wrapper object [sc-48362] * Update with changes from master [sc-48362] Merges in updates from master and resolves conflicts from relocation of some of the Query TypeScript definitions into core * Removes logic setting user offline and relying on results panel error message [sc-48362] * Fixes bad import after some TypeScript definitions were relocated to core [sc-48362] * Fixes TypeScript errors [sc-48362]

git commit -m 'Convert QueryAutoRefresh to functional component [sc-4…

dc45703

…8362]'

pull-request-size bot added the size/XL label May 24, 2022