[SIP-4] replace chart ajax calls with SupersetClient
#5875
Conversation
williaster
force-pushed
the
chris--ajax-charts
branch
from
0f631e8 to
e56f38d
Compare
williaster
force-pushed
the
chris--ajax-charts
branch
from
e56f38d to
2318918
Compare
| // of the passed response blob. It assumes the blob should be read as text, | ||
| // and that the response can be parsed as JSON. This is needed to read | ||
| // the value of any fetch-based response. | ||
| export default function readResponseBlob(blob) { |
There was a problem hiding this comment.
Can you double check cypress doesn't add this in as a test? If you run tests just make sure this file doesn't show up. I had trouble adding helpers / utils because it kept picking them up as tests. I'm not sure if it will happen because this is outside of the integration folder, but if it does I added a setting to ignore any files with the pattern *.helper.js.
There was a problem hiding this comment.
@michellethomas thanks for bringing this up, it looks okay from the cypress UI
williaster
force-pushed
the
chris--ajax-charts
branch
from
95f1d44 to
ea7a156
Compare
Codecov Report
@@ Coverage Diff @@
## master #5875 +/- ##
=======================================
Coverage 77.28% 77.28%
=======================================
Files 47 47
Lines 9332 9332
=======================================
Hits 7212 7212
Misses 2120 2120Continue to review full report at Codecov.
|
| export const CHART_UPDATE_STARTED = 'CHART_UPDATE_STARTED'; | ||
| export function chartUpdateStarted(queryRequest, latestQueryFormData, key) { | ||
| return { type: CHART_UPDATE_STARTED, queryRequest, latestQueryFormData, key }; | ||
| export function chartUpdateStarted(queryController, latestQueryFormData, key) { |
There was a problem hiding this comment.
noting that the abort controller doesn't seem to be passed along in the reducer
superset/assets/src/common.js
Outdated
| // this allows for the server side generated menus to function | ||
| window.$ = $; | ||
| window.jQuery = $; | ||
| require('bootstrap'); | ||
|
|
||
| SupersetClient.configure({ host: (window.location && window.location.host) || '' }) |
There was a problem hiding this comment.
noting from testing in staging that we must also configure protocol here
williaster
force-pushed
the
chris--ajax-charts
branch
from
6dd70b3 to
da7d5f1
Compare
superset/assets/src/chart/Chart.jsx
Outdated
| @@ -166,18 +167,38 @@ class Chart extends React.PureComponent { | |||
| ); | |||
| } | |||
|
|
|||
| d3format(col, number) { | |||
There was a problem hiding this comment.
Did you accidentally revert this file?
williaster
force-pushed
the
chris--ajax-charts
branch
from
8caf47d to
0265386
Compare
|
@kristw @michellethomas I tested this on staging (charts + dashboards) and it worked 🎉, rebased on newest master so that I could update unit tests to jest and I fixed newer cypress tests. I also fixed the Chart rebase-revert Krist pointed out so it should be good to go 👍 🤞 |
* [deps] add @superset-ui/core * [superset-client] initialize SupersetClient in app setup * [superset-client] add abortcontroller-polyfill * [superset-client] replace all chart ajax calls with SupersetClient * [tests] add fetch-mock dep and helpers/setupSupersetClient.js * [superset client][charts][tests] fix and improve chartActions_spec * [deps] @superset-ui/core@^0.0.4 * [common] add better SupersetClient initialization error * [cypress] add readResponseBlob helper, fix broken fetch-based tests * [cypress] fix tests from rebase * [deps] @superset-ui/core@^0.0.5 * [cypress][fetch] fix controls test for fetch * [cypress][dashboard][fetch] fix filter test for fetch * [superset-client] configure protocol on init * yarn.lock * undo Chart.jsx revert * yarn again * [superset-client] fix chartAction unit tests
mistercrunch
added
🏷️ bot
This PR is one of a few that implements the final step 4) discussed in #5772 and refactors just chart-specific ajax calls for easier review.
It also improves the JS tests for
chartActionswhich asserted only one thing previously.Note that the new
@superset-ui/coredep + setup forSupersetClientis duplicated across all ajax PRsSupersetClient#5854SupersetClient#5869SupersetClient#5896@kristw @mistercrunch @graceguo-supercat @michellethomas @conglei