-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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][superset-client] add SupersetClient
, refactor app
#5772
Conversation
@@ -26,7 +27,7 @@ | |||
"no-mixed-operators": 0, | |||
"no-continue": 0, | |||
"no-bitwise": 0, | |||
"no-undef": 0, | |||
"no-undef": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning for this, turning this off is BAD!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one total worths its own PR.
SupersetClient
, refactor appSupersetClient
, refactor app
@@ -42,7 +42,7 @@ describe('sliceEntities reducer', () => { | |||
{}, | |||
{ | |||
type: FETCH_ALL_SLICES_FAILED, | |||
error: { responseJSON: { message: 'errorrr' } }, | |||
error: 'errorrr', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a test / misspelling was purposeful.
import $ from 'jquery'; | ||
import sinon from 'sinon'; | ||
import fetchMock from 'fetch-mock'; | ||
import thunk from 'redux-thunk'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) love thunk
Okay so there are some concerns that this PR is too large and so we're considering the following:
|
Abandoning for described approach |
This PR implements SIP-4, replacing all JS ajax calls with a new
SupersetClient
that handles all async requests usingfetch
🎉It's a big PR due to the ajax refactoring, so here's an higher-altitude summary:
SupersetClient
usage**this is currently in
src/packages/core/src
which will become@superset-ui/core
SupersetClient
handles:CSRF
token authenticationSupersetClient.isAuthorized()
AbortController
polyfill)protocol = 'http'
host
headers
credentials = 'same-origin'
(include
for non-Superset apps)mode = 'same-origin'
(cors
for non-Superset apps)timeout
get
s andpost
s. I considered addingput
s/delete
s but no existing ajax calls of this type are currently madeurl
orendpoint
headers
body
timeout
signal
(for aborting, fromconst { signal } = (new AbortController())
)postPayload
(key values are added to anew FormData()
)stringify
whether to callJSON.stringify
onpostPayload
valuesNew dependencies
It introduces the following new packages:
abortcontroller-polyfill
whatwg-fetch
fetch-mock
for testingHow was this tested?
SupersetClient
testsNext steps
packages/core
tosuperset-ui
repo@superset-ui/core
TODO
SupersetClient
SupersetClient
/fetch
SupersetClient
methods likeput
anddestroy
SupersetClient
tests@mistercrunch @kristw @conglei @graceguo-supercat @hughhhh @betodealmeida @michellethomas