[SIP-4] add lerna monorepo and`@superset-ui/core` package with `SupersetClient` #1

williaster · 2018-09-06T05:29:21Z

This PR

sets up the superset-ui monorepo (i.e., a repository of multiple packages) with lerna and
adds the first package @superset-core, which implements SupersetClient as described in SIP-4. SupersetClient is pulled from step 2 this PR, which was abandoned for smaller steps, but supports all the client-side async functionality encompassed by the current Superset app.

Below are some more details about the monorepo + @superset-ui/core package, see the README (link soon) for API/usage details for the SupersetClient.

Monorepo overview

lerna is used to manage versions and dependencies between
packages in this monorepo.

superset-ui/
  lerna.json
  package.json
  ...
  packages/
    package1/
      package.json
      ...
      src/
      test/
      ...
      lib/
      esm/
      ...
    ...

To develop:

clone this repo
install the root npm modules including lerna and yarn
have lerna install package dependencies and manage the symlinking between packages for you

git clone ...superset-ui && cd superset-ui
npm install
lerna bootstrap

After this you can run scripts across the entire monorep (e.g., yarn run test runs tests across all repos) or within a given package. The latter means that each package defines its own build config, linting, and testing. I initially set it up to have shared things like jest, prettier, and linting but I think this will be more painful down the line. I think we can achieve consistency with yeoman package generators which @kristw and I will do in a future PR.

@superset-ui/core

After this PR is merged, we can publish the first version of this package.

@data-ui/build-config is used to manage all babel, jest testing, eslint, prettier. This type of "shareable" build config will save a lot of copy/paste as the # of packages grows.

Testing

We should have good coverage for client-side code from the start 🔥 In a future PR we'll set up code coverage but here are the stats for what I wrote 🚀

=== Coverage summary ===                                                                                                                
Statements   : 98.82% ( 84/85 )
Branches     : 90% ( 81/90 )
Functions    : 100% ( 34/34 )
Lines        : 98.78% ( 81/82 )
=====================

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

john-bodley · 2018-09-06T05:46:54Z

.gitignore

@@ -0,0 +1,30 @@
+# Logs


Personally I would just enumurate these in a single alphabetical list as it’s easier to process and maintain.

john-bodley · 2018-09-06T05:50:34Z

PULL_REQUEST_TEMPLATE.md

@@ -0,0 +1,9 @@
+💔 Breaking Changes


Can this be added to the .github folder?

maybe! I'll give it a shot

kristw · 2018-09-06T23:05:56Z

packages/superset-ui-core/src/SupersetClient.js

+      timeout: this.timeout,
+      url: this.getUrl({ endpoint: 'superset/csrf_token/', host: this.host }),
+    })
+      .then(response => {


Perhaps use .then(onFulfilled, onRejected)
instead of .then(onFulfilled).catch(onRejected)

.catch(onRejected) is equivalent to.then(undefined, onRejected)

The current catch clause will handle Promise.reject thrown from the then clause as well, which has the same result in this case, but is redundant work.

sure, I think the .catch is a little more readable but don't have a super strong opinion.

kristw · 2018-09-06T23:23:40Z

packages/superset-ui-core/src/SupersetClient.js

+  }
+
+  getCSRFToken() {
+    this.requestingCsrf = true;


Instead of having boolean flags this.requestingCsrf, we can store the promise returned from callApi() in this.requestingCsrf. The initial value of this.requestingCsrf can be Promise.reject('not initialized').

This may remove the need for this.waitForCSRF and polling/setTimeout mechanism as this.ensureAuth can return this.requestingCsrf; directly.

that's a great idea, I'll try to move toward that 👍

kristw · 2018-09-06T23:43:52Z

packages/superset-ui-core/src/callApi.js

+  return Promise.race([
+    new Promise((resolve, reject) => {
+      if (typeof timeout === 'number') {
+        timeoutId = setTimeout(


Perhaps create another function called createTimer.

function createTimer(timeout) { return new Promise((resolve, reject) => { setTimeout(() => reject(...), timeout); }); }

To avoid worrying about clearing timeout or passing around timeoutId, can break the callApi function into two parts

function callApiWithoutParsing() { // the first half of original callApi() // handle params and return fetch(); } function parseResponse(apiPromise) { return apiPromise.then( .... ).then( .... ); // do the parsing } function callApi() { return parseResponse(callApiWithoutParsing()); }

If timeout is not a number, there is no need to race.

function callApiWithTimeout() { const apiPromise = callApiWithoutParsing(...); // This will race until results come back from server // excluding parsing time const racedPromise = (typeof timeout === 'number') ? Promise.race([createTimer(timeout), apiPromise]) : apiPromise; return parseResponse(racedPromise); }

done, my only concern here is what happens to the timeouts without clearing them (when say a dashboard makes 15 requests for chart data). I couldn't find much info online about this, assume they're garbage collected.

williaster · 2018-09-07T00:40:44Z

packages/superset-ui-core/src/callApi.js

+        });
+      }
+
+      return Promise.resolve(typeof text === 'undefined' ? { json, response } : { response, text });


I think this resolve is extraneous, will remove. the rejection above is useful to forward the error.

… spys

williaster · 2018-09-07T02:15:47Z

packages/superset-ui-core/test/SupersetClient.test.js

@@ -0,0 +1,497 @@
+/* eslint promise/no-callback-in-promise: 'off' */
+import sinon from 'sinon';


just learned more about jest spys, should be able to move this

kristw · 2018-09-07T05:07:32Z

packages/superset-ui-core/README.md

+SupersetClient.get({}).then(...).catch(...);
+
+if (IWantToCancelForSomeReason) {
+  signal.abort(); // Promise is rejected, request `catch` is invoked


Does it cancel every outgoing request? Is there a way to cancel just one?

oops I forgot an important part of this usage (I'll update), you pass the signal to the request, so it's per-request aborting.

see here for an example in superset.

kristw · 2018-09-07T05:08:32Z

packages/superset-ui-core/src/callApi/callApiAndParseWithTimeout.js

+  const apiPromise = callApi(rest);
+
+  const racedPromise =
+    typeof timeout === 'number'


&& timeout > 0

kristw · 2018-09-07T05:09:35Z

packages/superset-ui-core/src/callApi/parseResponse.js

+          return typeof text === 'undefined' ? { json, response } : { response, text };
+        }),
+    // forward the error
+    error => Promise.reject(error),


This line is probably not necessary as error propagate through already

linting has complained is some places that we need a catch for every then, but it doesn't here 🤷‍♀️

kristw · 2018-09-07T05:12:48Z

packages/superset-ui-core/test/callApi/callApi.test.js

+
+  const mockGetPayload = { get: 'payload' };
+  const mockPostPayload = { post: 'payload' };
+  const mockErrorPayload = { status: 500, statusText: 'Internal errorz!' };


errorz. typo?

kristw · 2018-09-07T05:21:44Z

packages/superset-ui-core/test/callApi/callApi.test.js

@@ -0,0 +1,156 @@
+/* eslint promise/no-callback-in-promise: 'off' */


Awesome for having tests for all of these.

learned a lot about jest!

kristw · 2018-09-07T20:52:39Z

packages/superset-ui-core/src/SupersetClient.js

+  }
+
+  get({ host, url, endpoint, mode, credentials, headers, body, timeout, signal }) {
+    return this.ensureAuth().then(() =>


What do you think about attaching an abort function to the returned promise from get and post?

const promise = this.ensureAuth().then(...); promise.abort = signal && () => signal.abort(); return promise;

So can use like this.

const myPromise = SupersetClient.get(...); if(wannaCancel && myPromise.abort) { myPromise.abort(); }

The concern is the function abort() will only be available to the original Promise returned from get/post and not on the chained then(), which may be not obvious to developer.

Another thing is instead of creating a signal from outside and passing it in as an argument, can we construct the signal in get/post or callApi so all calls get abort functionality by default?

hmm, that's an interesting thought, the abort controller api is a little verbose. I do think it would be non-ideal to have the abort just attached to the initial promise.

maybe we can see how it goes with signal and we could develop a nicer / more complex api around aborting in the future? there were only a couple of places in Superset that use this currently.

in practice you either have to pass the abort controller around, or the promise, and so I'm not sure there's much advantage of one over the other except that the former is full-fledged API spec not just our library API.

sgtm. We can try that later. I think this first version looks good to me.

kristw

Overall lgtm. Left one question for the getCSRFToken() but not blocking.

kristw · 2018-09-07T21:42:12Z

packages/superset-ui-core/src/SupersetClient.js

+  }
+
+  get({ host, url, endpoint, mode, credentials, headers, body, timeout, signal }) {
+    return this.ensureAuth().then(() =>


sgtm. We can try that later. I think this first version looks good to me.

kristw · 2018-09-07T21:46:42Z

packages/superset-ui-core/test/callApi/rejectAfterTimeout.test.js

+import rejectAfterTimeout from '../../src/callApi/rejectAfterTimeout';
+import throwIfCalled from '../utils/throwIfCalled';
+
+describe('rejectAfterTimeout', () => {


nit
Add () in describe('rejectAfterTimeout()') so we know it is a function when reading test result

kristw · 2018-09-07T21:47:38Z

packages/superset-ui-core/test/callApi/parseResponse.test.js

+import { LOGIN_GLOB } from '../fixtures/constants';
+import throwIfCalled from '../utils/throwIfCalled';
+
+describe('parseResponse', () => {


nit
Add () in describe('parseResponse()') so we know it is a function when reading test result

kristw · 2018-09-07T21:48:39Z

packages/superset-ui-core/test/callApi/callApiAndParseWithTimeout.test.js

+import { LOGIN_GLOB } from '../fixtures/constants';
+import throwIfCalled from '../utils/throwIfCalled';
+
+describe('callApiAndParseWithTimeout', () => {


nit
Add () in describe('callApiAndParseWithTimeout()') so we know it is a function when reading test result

kristw · 2018-09-07T21:49:47Z

packages/superset-ui-core/test/callApi/callApi.test.js

+import { LOGIN_GLOB } from '../fixtures/constants';
+import throwIfCalled from '../utils/throwIfCalled';
+
+describe('callApi', () => {


nit
Add () in describe('callApi()') so we know it is a function when reading test result

kristw · 2018-09-07T21:55:07Z

packages/superset-ui-core/src/SupersetClient.js

+          return Promise.reject({ error: 'Failed to fetch CSRF token' });
+        }
+
+        return response;


Should this return response or this.csrfToken?
Nobody is really using its result at this point, but seems like this.csrfToken will be more expected?

If somebody called this multiple times, it will send new request every time. Is that expected behavior? or if a promise already exists, then return this.csrfPromise so it will send request only once.

…#1) fix: tooltip disappearance and stickiness

williaster added 2 commits September 5, 2018 17:13

[core] add lerna boilerplate + core package

a6e7fe9

[core][tests] cleaner promise tests

0ffdf03

john-bodley reviewed Sep 6, 2018

View reviewed changes

kristw reviewed Sep 6, 2018

View reviewed changes

williaster commented Sep 7, 2018

View reviewed changes

williaster added 3 commits September 6, 2018 19:09

[monorepo] move PR template into .github/

aa8d153

[monorepo] sort .gitignore entries

c512518

[core] refactor callApi into multiple pieces, use jest over sinon for…

984c865

… spys

williaster commented Sep 7, 2018

View reviewed changes

kristw reviewed Sep 7, 2018

View reviewed changes

[core] use Promise-based queuing in SupersetClient

ae94774

kristw reviewed Sep 7, 2018

View reviewed changes

williaster added 3 commits September 6, 2018 22:40

[core] use jest for spys not sinon

1ef6da5

[core] update aborting details in readme + misc fixes

a89cb0e

[core] fix broken timeout test

49f9c54

kristw reviewed Sep 7, 2018

View reviewed changes

[core] fix linting

b3ffc4e

kristw approved these changes Sep 7, 2018

View reviewed changes

[core][tests] clarify function() calls in test names

1fc92cf

williaster merged commit 7620154 into master Sep 8, 2018

soboko mentioned this pull request Jan 8, 2019

Handle BigNumber conversions in JSON properly (without loss of precision) #71

Merged

kristw pushed a commit that referenced this pull request Apr 17, 2020

fix(legacy-preset-chart-nvd3): tooltip's disappearance and stickiness (…

d1a2a59

…#1) fix: tooltip disappearance and stickiness

xiezhongfu mentioned this pull request May 15, 2021

fix(plugin-chart-echarts): funnel chart improvements #1110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIP-4] add lerna monorepo and`@superset-ui/core` package with `SupersetClient` #1

[SIP-4] add lerna monorepo and`@superset-ui/core` package with `SupersetClient` #1

williaster commented Sep 6, 2018

john-bodley Sep 6, 2018

john-bodley Sep 6, 2018

williaster Sep 6, 2018

kristw Sep 6, 2018

williaster Sep 7, 2018

kristw Sep 6, 2018 •

edited

Loading

williaster Sep 7, 2018

kristw Sep 6, 2018

williaster Sep 7, 2018

williaster Sep 7, 2018

williaster Sep 7, 2018

kristw Sep 7, 2018

williaster Sep 7, 2018

kristw Sep 7, 2018

kristw Sep 7, 2018

williaster Sep 7, 2018

kristw Sep 7, 2018

kristw Sep 7, 2018

williaster Sep 7, 2018

kristw Sep 7, 2018 •

edited

Loading

williaster Sep 7, 2018

williaster Sep 7, 2018

kristw Sep 7, 2018

kristw left a comment

kristw Sep 7, 2018

kristw Sep 7, 2018

kristw Sep 7, 2018

kristw Sep 7, 2018

kristw Sep 7, 2018

kristw Sep 7, 2018

		@@ -0,0 +1,497 @@
		/* eslint promise/no-callback-in-promise: 'off' */
		import sinon from 'sinon';

		@@ -0,0 +1,156 @@
		/* eslint promise/no-callback-in-promise: 'off' */

[SIP-4] add lerna monorepo and@superset-ui/core package with SupersetClient #1

[SIP-4] add lerna monorepo and@superset-ui/core package with SupersetClient #1

Conversation

williaster commented Sep 6, 2018

Monorepo overview

@superset-ui/core

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[SIP-4] add lerna monorepo and`@superset-ui/core` package with `SupersetClient` #1

[SIP-4] add lerna monorepo and`@superset-ui/core` package with `SupersetClient` #1

kristw Sep 6, 2018 •

edited

Loading

kristw Sep 7, 2018 •

edited

Loading