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

[SIP-4] replace dashboard ajax calls with SupersetClient #5854

Merged
merged 3 commits into from
Oct 16, 2018

Conversation

williaster
Copy link
Contributor

@williaster williaster commented Sep 11, 2018

This PR is one of a few PRs that implement the final step 4) discussed in #5772, to refactor just dashboard-specific ajax calls (not including charts) for easier review

Note that the new @superset-ui/core dep + setup for SupersetClient is duplicated across all ajax PRs:

@kristw @mistercrunch @graceguo-supercat @michellethomas @conglei

@codecov-io
Copy link

codecov-io commented Sep 11, 2018

Codecov Report

Merging #5854 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5854   +/-   ##
=======================================
  Coverage   77.28%   77.28%           
=======================================
  Files          47       47           
  Lines        9332     9332           
=======================================
  Hits         7212     7212           
  Misses       2120     2120

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 91792a5...c389e77. Read the comment docs.

@graceguo-supercat
Copy link

graceguo-supercat commented Sep 19, 2018

I do have some concerns for this PR:

  • It will replace jquery.ajax with a new toolkit (based on fetch, added helper and wrapper) in all Superset.
    Is there any existed Superset dependencies that depends on jquery?

  • This toolkit lives outside Superset codebase.
    Given it didn't have any mature customers yet, I feel we (Superset developers) may hit into its bugs or add improvement pretty often. Superset frontend is ajax-heavy. For the long term, when we develop some new features (like improve logging or batch-fetch etc, maybe?), I need to consider should i add this additional function in Superset or in the toolkit code base?
    When we need a little change on FAB, I need to fix it in another codebase, tested it in another codebase with many context i am not familiar, wait it build and generate a new version, then i can use it in Superset. This toolkit will be much more friendly than FAB, but the repo is different :)

  • Is it possible that make the new toolkit inside Superset, but generate standalone modules, to share with embeddable chart?

I guess all above are somewhat discussed in the meeting. If all of our major contributors (especially frontend contributors) accept inconvenience, i can accept it too :)

@mistercrunch @betodealmeida @hughhhh @xtinec @JamshedRahman @jeffreythewang @fabianmenges @michellethomas @williaster @kristw @conglei

@williaster
Copy link
Contributor Author

good comments, @graceguo-supercat. Several good questions in there, I added my response to the actual SIP-4 here.

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM

@williaster williaster merged commit 462c58e into master Oct 16, 2018
@williaster williaster deleted the chris--ajax-dashboard branch October 16, 2018 20:42
bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018
* [core] replace dashboard ajax calls with SupersetClient

* [core] fix SupersetClient dashboard tests

* [dashboard][superset-client] don't error by parsing save dashboard response as json
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 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 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants