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][superset-client] add SupersetClient, refactor app #5772

Closed
wants to merge 9 commits into from

Conversation

williaster
Copy link
Contributor

@williaster williaster commented Aug 29, 2018

This PR implements SIP-4, replacing all JS ajax calls with a new SupersetClient that handles all async requests using fetch 🎉

It's a big PR due to the ajax refactoring, so here's an higher-altitude summary:

SupersetClient usage*

// appSetup.js
import SupersetClient from `@superset-ui/core`;

SupersetClient.configure(...clientConfig);
SupersetClient.init(); // CSRF auth, can also chain `.configure().init();

// anotherFile.js
import SupersetClient from `@superset-ui/core`;

SupersetClient.post(...requestConfig)
  .then(({ request, json }) => ...)
  .catch((error) => ...);

*this is currently in src/packages/core/src which will become @superset-ui/core

SupersetClient handles:

  • CSRF token authentication
    • it also queues requests in the case that another request is made before the token is received
    • it checks for a token before every request, an external app that uses this can detect this by catching errors, or explicitly checking SupersetClient.isAuthorized()
  • timeouts (not supported by default in fetch)
  • query aborts (also not supported by default in fetch, uses AbortController polyfill)
  • supports the following configuration options:
    • protocol = 'http'
    • host
    • headers
    • credentials = 'same-origin' (include for non-Superset apps)
    • mode = 'same-origin' (cors for non-Superset apps)
    • timeout
  • supports gets and posts. I considered adding puts/deletes but no existing ajax calls of this type are currently made
  • a given request supports the following options, which met the need of every async call in the app:
    • url or endpoint
    • headers
    • body
    • timeout
    • signal (for aborting, from const { signal } = (new AbortController()))
    • for posts
      • postPayload (key values are added to a new FormData())
      • stringify whether to call JSON.stringify on postPayload values

New dependencies

It introduces the following new packages:

  • abortcontroller-polyfill
  • whatwg-fetch
  • fetch-mock for testing

How was this tested?

  • attempted to test success + failure of every endpoint that was refactored in the browser
  • updated broken / ajax-containing tests
  • (TODO) add SupersetClient tests

Next steps

  1. finish todos
  2. merge this PR, test in staging, fix any immediate bugs
  3. move packages/core to superset-ui repo
  4. update imports to pull from @superset-ui/core

TODO

  • implement SupersetClient
  • refactor all ajax calls to use SupersetClient / fetch
  • refactor / fix all ajax-containing tests
  • debug SQL lab polling
  • remove unused SupersetClient methods like put and destroy
  • refactor API
  • rebase
  • add SupersetClient tests
  • add API documentation to package readme
  • verify sqllab error links

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

@@ -26,7 +27,7 @@
"no-mixed-operators": 0,
"no-continue": 0,
"no-bitwise": 0,
"no-undef": 0,
"no-undef": 1,
Copy link
Contributor Author

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

+100

Copy link
Contributor

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.

@williaster williaster changed the title [superset-client] add SupersetClient, refactor app [SIP-4][superset-client] add SupersetClient, refactor app Aug 29, 2018
@@ -42,7 +42,7 @@ describe('sliceEntities reducer', () => {
{},
{
type: FETCH_ALL_SLICES_FAILED,
error: { responseJSON: { message: 'errorrr' } },
error: 'errorrr',
Copy link
Member

@hughhhh hughhhh Aug 29, 2018

Choose a reason for hiding this comment

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

nit: error

Copy link
Contributor Author

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';
Copy link
Member

Choose a reason for hiding this comment

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

:) love thunk

@williaster
Copy link
Contributor Author

Okay so there are some concerns that this PR is too large and so we're considering the following:

  1. abandon this PR
  2. create a PR in the superset-ui repo with just the SupersetClient package
  3. merge then publish the package
  4. create a series of smaller PRs for the ajax refactors, possibly adding integration tests to them as we go.

@williaster
Copy link
Contributor Author

Abandoning for described approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants