Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

[core] add parseMethod #5

Merged
merged 9 commits into from
Sep 18, 2018
+319 −300
Merged

[core] add parseMethod #5

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
21b5074
[core] expose 'parseMethod' in SupersetClient and callApi for text re…
williaster Sep 13, 2018
397240e
[parseResponse][test] fix parseMethod test
williaster Sep 13, 2018
560f30b
[monorepo] gitignore .npmrc
williaster Sep 13, 2018
b01e770
[core][deps] @data-ui/build-config => ^0.0.14
williaster Sep 13, 2018
e138fc2
[monorepo] tweak npm scripts
williaster Sep 13, 2018
a19871c
[core] clean up parseResponse
williaster Sep 13, 2018
c8c3558
[core] clean up parseResponse more
williaster Sep 13, 2018
fc7e7db
[core] don't fallback to text parsing if response is supposed to be j…
williaster Sep 18, 2018
85a1bdd
[core][tests] fix borken tests, return promises instead of calling do…
williaster Sep 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
.eslintrc.js
.idea
.npm
.npmrc
.prettierignore
.yarnclean

Expand Down
5 changes: 2 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@
"private": true,
"scripts": {
"build": "lerna run build",
"jest": "lerna run test",
"lint": "lerna run lint",
"lint:fix": "lerna run lint:fix",
"prerelease": "yarn run build",
"prepare-release": "git checkout master && git pull --rebase origin master && yarn run test",
"prepare-release": "git checkout master && git pull --rebase origin master && lerna bootstrap && yarn run lint && yarn run test",
"release": "yarn run prepare-release && lerna publish && lerna run gh-pages",
"test": "lerna bootstrap && yarn run lint && yarn run jest"
"test": "lerna run test"
},
"repository": "https://github.com/apache-superset/superset-ui.git",
"keywords": [
Expand Down
2 changes: 1 addition & 1 deletion packages/superset-ui-core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
},
"homepage": "https://github.com/apache-superset/superset-ui#readme",
"devDependencies": {
"@data-ui/build-config": "^0.0.12",
"@data-ui/build-config": "^0.0.14",
"fetch-mock": "^6.5.2",
"node-fetch": "^2.2.0"
},
Expand Down
13 changes: 8 additions & 5 deletions packages/superset-ui-core/src/SupersetClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,15 @@ class SupersetClient {
);
}

get({ host, url, endpoint, mode, credentials, headers, body, timeout, signal }) {
get({ body, credentials, headers, host, endpoint, mode, parseMethod, signal, timeout, url }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It starts to get long, do you want to separate into one param per line?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh just notice post is already doing that, then maybe good to spread this one out too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's all based on prettier config, it auto wraps if it's over the 100 char limit (which post is but get is not). we could decrease the char limit if you feel strongly about it tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

nope. this is pretty minor. I'll leave it to you.

return this.ensureAuth().then(() =>
callApi({
body,
credentials: credentials || this.credentials,
headers: { ...this.headers, ...headers },
method: 'GET',
mode: mode || this.mode,
parseMethod,
signal,
timeout: timeout || this.timeout,
url: url || this.getUrl({ endpoint, host: host || this.host }),
Expand All @@ -91,23 +92,25 @@ class SupersetClient {
}

post({
credentials,
headers,
host,
endpoint,
url,
mode,
credentials,
headers,
parseMethod,
postPayload,
timeout,
signal,
stringify,
timeout,
url,
}) {
return this.ensureAuth().then(() =>
callApi({
credentials: credentials || this.credentials,
headers: { ...this.headers, ...headers },
method: 'POST',
mode: mode || this.mode,
parseMethod,
postPayload,
signal,
stringify,
Expand Down
1 change: 0 additions & 1 deletion packages/superset-ui-core/src/callApi/callApi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ export default function callApi({
postPayload,
stringify = true,
redirect = 'follow', // manual, follow, error
timeoutId,
signal, // used for aborting
}) {
let request = {
Expand Down
4 changes: 2 additions & 2 deletions packages/superset-ui-core/src/callApi/callApiAndParseWithTimeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ import callApi from './callApi';
import rejectAfterTimeout from './rejectAfterTimeout';
import parseResponse from './parseResponse';

export default function callApiAndParseWithTimeout({ timeout, ...rest }) {
export default function callApiAndParseWithTimeout({ timeout, parseMethod, ...rest }) {
const apiPromise = callApi(rest);

const racedPromise =
typeof timeout === 'number' && timeout > 0
? Promise.race([rejectAfterTimeout(timeout), apiPromise])
: apiPromise;

return parseResponse(racedPromise);
return parseResponse(racedPromise, parseMethod);
}
43 changes: 23 additions & 20 deletions packages/superset-ui-core/src/callApi/parseResponse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,29 @@
export default function parseResponse(apiPromise) {
return apiPromise.then(apiResponse =>
const PARSERS = {
json: response =>
// first try to parse as json, and fall back to text (e.g., in the case of HTML stacktrace)
// cannot fall back to .text() without cloning the response because body is single-use
apiResponse
response
.clone()
.json()
.catch(() => /* jsonParseError */ apiResponse.text().then(textPayload => ({ textPayload })))
.then(maybeJson => ({
json: maybeJson.textPayload ? undefined : maybeJson,
response: apiResponse,
text: maybeJson.textPayload,
}))
.then(({ response, json, text }) => {
if (!response.ok) {
return Promise.reject({
error: response.error || (json && json.error) || text || 'An error occurred',
status: response.status,
statusText: response.statusText,
});
}
.then(json => ({ json, response }))
.catch(() => /* jsonParseError */ response.text().then(text => ({ response, text }))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean the developer always have to check if output.json exists or not? If output.json does not exist, they should use to output.text instead. If they forget to check. This can cause bugs. I wonder if there is a way to make it more explicit.

Also, generally, does one expect this Promise to reject when fail to parse or resolve text of the supposed-to-be json? IMO, I would expect it to fail and give me the text in the error message for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah now that we expose the parse method and are explicit about text/json we should probably not clone and fallback to text gracefully, and instead it should probably throw.

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 broke a lot of tests, which were falling back to .text() unknowingly because of my mock text response 😜


return typeof text === 'undefined' ? { json, response } : { response, text };
}),
);
text: response => response.text().then(text => ({ response, text })),
};

export default function parseResponse(apiPromise, parseMethod = 'json') {
const responseParser = PARSERS[parseMethod] || PARSERS.json;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can developer pass parseMethod as a custom function in addition to string of predefined parser name?

Copy link
Contributor Author

@williaster williaster Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about that but am not sure a free-form parser makes sense. There are only a few available on the Response object, of which we are exposing text and json.

It doesn't seem worthwhile to copy / mirror the full response API to me (would begin questioning the need for this lib). One option could be to allow null or 'none' as a parseMethod to enable returning the response unmodified and un-read. Beyond that, any custom parsing I think should come from a chained .then(({ json }) => myCustomParser(json))

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm


return apiPromise.then(responseParser).then(({ json, response, text }) => {
// HTTP 404 or 500 are not rejected, ok is just set to false
if (!response.ok) {
return Promise.reject({
error: response.error || (json && json.error) || text || 'An error occurred',
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps should separate error handling for apiResponse, json parsing and text parsing.

Copy link
Contributor

Choose a reason for hiding this comment

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

return apiPromise
  .then((response) => {
    if (!response.ok) {
      return Promise.reject({
        error: response.error,
        status: response.status,
        statusText: response.statusText,
      });
    }
    return response;
  })
  .then(responseParser)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dig the latter^

status: response.status,
statusText: response.statusText,
});
}

return { json, response, text };
});
}
43 changes: 43 additions & 0 deletions packages/superset-ui-core/test/SupersetClient.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,11 +243,16 @@ describe('SupersetClient', () => {
const host = 'HOST';
const mockGetEndpoint = '/get/url';
const mockPostEndpoint = '/post/url';
const mockTextEndpoint = '/text/endpoint';
const mockGetUrl = `${protocol}://${host}${mockGetEndpoint}`;
const mockPostUrl = `${protocol}://${host}${mockPostEndpoint}`;
const mockTextUrl = `${protocol}://${host}${mockTextEndpoint}`;
const mockTextJsonResponse = '{ "value": 9223372036854775807 }';

fetchMock.get(mockGetUrl, 'Ok');
fetchMock.post(mockPostUrl, 'Ok');
fetchMock.get(mockTextUrl, mockTextJsonResponse);
fetchMock.post(mockTextUrl, mockTextJsonResponse);

it('checks for authentication before every get and post request', done => {
expect.assertions(3);
Expand Down Expand Up @@ -321,6 +326,25 @@ describe('SupersetClient', () => {
.catch(throwIfCalled);
});

it('supports parsing a response as text', done => {
expect.assertions(2);
const client = new SupersetClient({ protocol, host });
client
.init()
.then(() =>
client
.get({ url: mockTextUrl, parseMethod: 'text' })
.then(({ text }) => {
expect(fetchMock.calls(mockTextUrl)).toHaveLength(1);
expect(text).toBe(mockTextJsonResponse);

return done();
})
.catch(throwIfCalled),
)
.catch(throwIfCalled);
});

it('allows overriding host, headers, mode, and credentials per-request', done => {
expect.assertions(3);
const clientConfig = {
Expand Down Expand Up @@ -418,6 +442,25 @@ describe('SupersetClient', () => {
.catch(throwIfCalled);
});

it('supports parsing a response as text', done => {
expect.assertions(2);
const client = new SupersetClient({ protocol, host });
client
.init()
.then(() =>
client
.post({ url: mockTextUrl, parseMethod: 'text' })
.then(({ text }) => {
expect(fetchMock.calls(mockTextUrl)).toHaveLength(1);
expect(text).toBe(mockTextJsonResponse);

return done();
})
.catch(throwIfCalled),
)
.catch(throwIfCalled);
});

it('passes postPayload key,values in the body', done => {
expect.assertions(3);

Expand Down
20 changes: 20 additions & 0 deletions packages/superset-ui-core/test/callApi/parseResponse.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,26 @@ describe('parseResponse()', () => {
.catch(throwIfCalled);
});

it('resolves to { text, response } if the parseMethod is set to `text`', done => {
expect.assertions(3);

// test with json + bigint to ensure that it was not first parsed as json
const mockTextParseUrl = '/mock/textparse/url';
const mockTextJsonResponse = '{ "value": 9223372036854775807 }';
fetchMock.get(mockTextParseUrl, mockTextJsonResponse);

const apiPromise = callApi({ url: mockTextParseUrl, method: 'GET' });
parseResponse(apiPromise, 'text')
.then(args => {
expect(fetchMock.calls(mockTextParseUrl)).toHaveLength(1);
expect(Object.keys(args)).toEqual(expect.arrayContaining(['response', 'text']));
expect(args.text).toBe(mockTextJsonResponse);

return done();
})
.catch(throwIfCalled);
});

it('rejects if the request throws', done => {
expect.assertions(3);

Expand Down