[core] add parseMethod

[williaster]

🏆 Enhancements

• This adds a parseMethod argument to parseResponse in order to support both json (default) and text responses. This is needed to enable custom response parsing in some cases, for example BigInts which was added to Sqllab recently.
• Exposes this parseMethod as an optional argument to SupersetClient.get() and .post() calls.

🏠 Internal

• Bumps @data-ui/build-config to 0.0.14 which has a stricter no-unused-args eslint rule
• adds .npmrc to .gitignore for local publishing credentials

@kristw @conglei @mistercrunch

[williaster]

codecov is stuck in processing, hopefully will post eventually

[kristw]

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

[kristw]

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

[williaster]

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.

[kristw]

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

[kristw]

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

[williaster]

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))

[kristw]

sgtm

[kristw]

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

[kristw]

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

[williaster]

I dig the latter^

[kristw]

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.

[williaster]

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.

[williaster]

this broke a lot of tests, which were falling back to .text() unknowingly because of my mock text response 😜

[williaster]

just noting that

• I updated tests to return a promise, which is an alternative for calling done()
• I removed unnecessary .catch(throwIfCalled) lines in tests because
  1. tests fail already with unhandled catchs and
  2. they hide errors that you might have in a then block

[williaster]

I will also fix the 90.9% of diff hit issue in another PR. there is still 98.5% coverage after this PR, I think the percentage of the lines changed in a PR is not as relevant and the threshold should be on total coverage %.