[kfetch] TypeScript-ify #20914

spalger · 2018-07-18T05:49:34Z

In order to make the awesome new kfetch api easier to consume in purely TypeScript projects, and since it's a pretty small module with very few dependencies, I converted it to TypeScript.

Along with kfetch I also started a type definition file for ui/chrome that we can extend as we go, but will likely be unnecessary after #19992

elasticmachine · 2018-07-18T08:48:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sorenlouv · 2018-07-18T12:16:03Z

src/ui/public/chrome/index.d.ts

+ * under the License.
+ */
+
+import Angular from 'angular';


I don't see Angular used here?

Yep, had another method typed but removed it because it was irrelevant

sorenlouv · 2018-07-18T12:17:52Z

src/ui/public/kfetch/kfetch.test.ts

 describe('kfetch', () => {
-  const matcherName = /my\/path/;
+  const matcherName: any = /my\/path/;


Can you use the RegExp type instead of any?

.. or actually, i'd just ditch the type. Typescript will infer it since it is static

The reason any is necessary is that the types for fetch-mock insist that the matcherName must be a string. By casting the variable to any we bypass the checks, and since the tests won't pass if the matcher isn't working correctly I feel okay about using any here.

sorenlouv · 2018-07-18T12:24:41Z

src/ui/public/kfetch/kfetch.ts

+      // attempt to read the body of the response
+      return reject(new FetchError(res, await res.json()));
+    } catch (err) {
+      // ignore error if we are not be able to get body for response


Perhaps update/remove this comment, since we no longer ignore the error.

Well, we still ignore the err variable

sorenlouv · 2018-07-18T12:43:56Z

src/ui/public/kfetch/kfetch_abortable.ts

@@ -29,7 +31,10 @@ function createAbortable() {
  };
 }

-export function kfetchAbortable(fetchOptions, kibanaOptions) {
+export function kfetchAbortable(
+  fetchOptions?: Omit<KFetchOptions, 'signal'>,


I'm probably reading this wrong but I don't understand the need for excluding signal. It's not part of KFetchOptions.

I'd expect fetchOptions: KFetchOptions to be fine - but maybe not?

KFetchOptions extends the default options for the global fetch API, which includes signal, which is why I've explicitly removed it from the type here (since we overwrite it)

I see that the RequestInit interface has signal of type AbortSignal. The signal we are overwriting kibanaOptions with is also of type AbortSignal - so should be compatible.

I pulled your branch and removed the Omit part, and it still compiles. Haven't worked with TS for a while so I might be missing something.

Do you get a compile error if you remove the Omit part?

Nope, the type described in the arguments should be the type that callers must match, it doesn't describe the type of the object we'll be passing to kfetch(). I'm just trying to remove the signal option so when a caller is looking at the options through intelisense or something they will see signal as an option for kfetch():

but not for kfetchAbortable():

Got it :) Thanks for the explanation.

sorenlouv · 2018-07-18T12:46:55Z

Unrelated to this: I noticed we have three fetch polyfills in OSS and x-pack: node-fetch, whatwg-fetch and isomorphic-fetch.
isomorphic-fetch works in both Node and the browser so perhaps we can ditch the two others? Alternatively we should ditch isomorphic-fetch.

EDIT: at a closer look isomorphic-fetch just uses the two others under the hood, and the versions are fairly outdated: https://github.com/matthew-andrews/isomorphic-fetch/blob/master/package.json#L22-L23

So isomorphic-fetch should probably go - but that's for another PR.

sorenlouv

A few nits and questions but overall looks really good. Awesome with types!

sorenlouv · 2018-07-18T22:00:28Z

src/ui/public/kfetch/kfetch.ts

+  prependBasePath?: boolean;
+}
+
+export function kfetch(fetchOptions?: KFetchOptions, kibanaOptions?: KFetchKibanaOptions) {


fetchOptions should probably be required. Are there any cases where it makes sense to call kfetch() (without args)?

Only if you want to do a get request to "/", I'm not sure why I made it optional... I guess because it would work fine if it wasn't specified. I'll update it anyway.

elasticmachine · 2018-07-18T22:45:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-19T05:01:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

In order to make the awesome new kfetch api easier to consume in purely TypeScript projects, and since it's a pretty small module with very few dependencies, I converted it to TypeScript. Along with kfetch I also started a type definition file for `ui/chrome` that we can extend as we go, but will likely be unnecessary after elastic#19992

In order to make the awesome new kfetch api easier to consume in purely TypeScript projects, and since it's a pretty small module with very few dependencies, I converted it to TypeScript. Along with kfetch I also started a type definition file for `ui/chrome` that we can extend as we go, but will likely be unnecessary after #19992

spalger · 2018-07-19T22:37:39Z

6.x/6.4: 801e5c0

@sqren

As @sqren mentioned in elastic#20914 (comment) we are using three different fetch polyfills, and it's not totally clear which one is winning out. For some time `import 'whatwg-fetch'` has been included globally in the browser for the last 6 months or so, but wasn't compatible with jest tests running in node.js, which seems to be the reason people have been reaching for `isomorphic-fetch`. Rather than relying on the version of `whatwg-fetch` and `node-fetch` that `isomorphic-fetch` depends on, we can just rely on them directly and stub out the global fetch for jest tests in a jest setup module.

spalger added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.4.0 labels Jul 18, 2018

spalger requested a review from sorenlouv July 18, 2018 05:49

[kfetch] TypeScript-ify

278f899

spalger force-pushed the ts/kfetch branch from d18d697 to 278f899 Compare July 18, 2018 05:51

spalger added 2 commits July 17, 2018 23:03

[kfetch] remove pointless return value typing

5cd5f33

[kfetch/tests] setup mocks before imports

c607cce

sorenlouv reviewed Jul 18, 2018

View reviewed changes

sorenlouv approved these changes Jul 18, 2018

View reviewed changes

spalger added 3 commits July 18, 2018 14:19

Merge branch 'master' of github.com:elastic/kibana into ts/kfetch

a344482

[ui/chrome/types] remove unused angular import

7f56055

[ui/kfetch] adapt comment to be more clear

5e62a68

sorenlouv reviewed Jul 18, 2018

View reviewed changes

[kfetch] don't make fetchOptions optional

3656e17

spalger mentioned this pull request Jul 19, 2018

[fetch] polyfill globally like all others #20963

Closed

sorenlouv approved these changes Jul 19, 2018

View reviewed changes

spalger merged commit b434652 into elastic:master Jul 19, 2018

spalger mentioned this pull request Jul 19, 2018

[6.x] [kfetch] TypeScript-ify (#20914) #20983

Merged

spalger deleted the ts/kfetch branch July 19, 2018 22:37

weltenwort mentioned this pull request Jul 23, 2018

[Infra UI] Remove obsolete axios-based field adapter #21091

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kfetch] TypeScript-ify #20914

[kfetch] TypeScript-ify #20914

spalger commented Jul 18, 2018

elasticmachine commented Jul 18, 2018

sorenlouv Jul 18, 2018

spalger Jul 18, 2018

sorenlouv Jul 18, 2018

sorenlouv Jul 18, 2018

spalger Jul 18, 2018

sorenlouv Jul 18, 2018

sorenlouv Jul 18, 2018

spalger Jul 18, 2018

sorenlouv Jul 18, 2018

spalger Jul 18, 2018

sorenlouv Jul 18, 2018 •

edited

Loading

spalger Jul 19, 2018 •

edited

Loading

sorenlouv Jul 19, 2018

sorenlouv commented Jul 18, 2018 •

edited

Loading

sorenlouv left a comment

sorenlouv Jul 18, 2018

spalger Jul 19, 2018

elasticmachine commented Jul 18, 2018

elasticmachine commented Jul 19, 2018

spalger commented Jul 19, 2018

[kfetch] TypeScript-ify #20914

[kfetch] TypeScript-ify #20914

Conversation

spalger commented Jul 18, 2018

elasticmachine commented Jul 18, 2018

💚 Build Succeeded

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

sorenlouv Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

spalger Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv commented Jul 18, 2018 • edited Loading

sorenlouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 18, 2018

💚 Build Succeeded

elasticmachine commented Jul 19, 2018

💚 Build Succeeded

spalger commented Jul 19, 2018

sorenlouv Jul 18, 2018 •

edited

Loading

spalger Jul 19, 2018 •

edited

Loading

sorenlouv commented Jul 18, 2018 •

edited

Loading