feat: Refactor asyncEvent middleware and add websocket support #13696
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13696 +/- ##
==========================================
- Coverage 77.43% 73.24% -4.20%
==========================================
Files 933 620 -313
Lines 47093 21985 -25108
Branches 5818 5731 -87
==========================================
- Hits 36465 16102 -20363
+ Misses 10485 5741 -4744
+ Partials 143 142 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
robdiciuccio
force-pushed
the
rd/async-event-refactor
branch
from
96bb118
to
7e5d039
Compare
robdiciuccio
force-pushed
the
rd/async-event-refactor
branch
from
7e5d039
to
d54831c
Compare
|
Awesome @robdiciuccio! Testing/reviewing this weekend. Quick observation: you need to upgrade to |
There was a problem hiding this comment.
I think this is a great improvement and makes the the async query framework much more approachable. I was able to make native filters work with this feature with just a few small changes (although a proper cleanup is probably in order there, too). I'm happy to implement this in native filters once this is merged. I left a few non-blocking comments, but LGTM (although please do update the lockfile to v2!).
| const chartData = { | ||
| some: 'data', |
There was a problem hiding this comment.
nit: This comes down to personal preference, but for me it's always easier to digest tests/mocks if they resemble real requests/responses as much as possible. In this case I believe chartData should be of type ChartDataResponseResult[]: https://github.com/apache-superset/superset-ui/blob/a3ec94f7a72a3dbb6cb6563e7ea7d3582661ea43/packages/superset-ui-core/src/query/types/QueryResponse.ts#L46-L72
| switch (asyncEvent.status) { | ||
| case JOB_STATUS.DONE: { | ||
| let { data, status } = await fetchCachedData(asyncEvent); // eslint-disable-line prefer-const | ||
| data = Array.isArray(data) ? data : [data]; |
There was a problem hiding this comment.
superset-ui/core has a convenient util for this: data = ensureIsArray(data): https://github.com/apache-superset/superset-ui/blob/a3ec94f7a72a3dbb6cb6563e7ea7d3582661ea43/packages/superset-ui-core/src/utils/ensureIsArray.ts#L24-L29
| componentId: number; | ||
| status: string; | ||
| data: any; | ||
| }; | ||
| type AppConfig = Record<string, any>; | ||
| type ListenerFn = (asyncEvent: AsyncEvent) => Promise<any>; |
There was a problem hiding this comment.
Bycatch: I believe the correct type for data should be LegacyQueryData | ChartDataResponseResult[]
There was a problem hiding this comment.
This could be a response payload for SQL Lab as well (in the future), which is the reason why it's generic. Well, that and laziness.
There was a problem hiding this comment.
Oh right, that makes total sense 👍
| "jsdom": "^16.4.0", | ||
| "less": "^3.12.2", | ||
| "less-loader": "^5.0.0", | ||
| "mini-css-extract-plugin": "^0.4.0", | ||
| "mock-socket": "^9.0.3", |
There was a problem hiding this comment.
Apparently it's a peer dependency of jest-websocket-mock.
mistercrunch
added
🏷️ bot
SUMMARY
Migrate asyncEvent module from Redux middleware to a Promise-based event listener approach (with @suddjian) and add Websocket event transport support (extracted from #11498).
Resolves an incompatibility between async queries and dashboard native filters.
TEST PLAN
Verify async queries function properly with polling with and without dashboard native filters enabled.
ADDITIONAL INFORMATION