From 8c972fec5060f185dfa3cd995946c717a944145e Mon Sep 17 00:00:00 2001 From: Eli Perelman Date: Fri, 3 May 2019 18:26:26 -0500 Subject: [PATCH] Remove abortable promise functionality --- src/core/public/http/abortable.test.ts | 49 --------- src/core/public/http/abortable.ts | 38 ------- src/core/public/http/fetch.ts | 91 ++++++++++++++++ src/core/public/http/http_service.test.ts | 11 -- src/core/public/http/http_service.ts | 101 +++--------------- src/core/public/http/types.ts | 5 - src/legacy/ui/public/kfetch/kfetch.test.ts | 7 -- src/legacy/ui/public/kfetch/kfetch.ts | 37 ++----- .../public/search/rollup_search_strategy.js | 8 +- 9 files changed, 118 insertions(+), 229 deletions(-) delete mode 100644 src/core/public/http/abortable.test.ts delete mode 100644 src/core/public/http/abortable.ts create mode 100644 src/core/public/http/fetch.ts diff --git a/src/core/public/http/abortable.test.ts b/src/core/public/http/abortable.test.ts deleted file mode 100644 index 0321e9ba3c50d61..000000000000000 --- a/src/core/public/http/abortable.test.ts +++ /dev/null @@ -1,49 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -// @ts-ignore -import fetchMock from 'fetch-mock/es5/client'; -import { HttpFetchOptions } from './types'; -import { abortable } from './abortable'; - -const Abortable = abortable((path: string, options?: HttpFetchOptions) => fetch('/', options)); - -describe('abortable', () => { - afterEach(() => { - fetchMock.restore(); - }); - - it('should return an abortable promise', async () => { - fetchMock.get('*', { status: 500 }); - - const promise = Abortable('/resolved'); - - expect(promise).toBeInstanceOf(Promise); - - promise.abort(); - - try { - await promise; - throw new Error('Unexpected Resolution'); - } catch (err) { - expect(err.message).not.toMatch(/Internal Server Error/); - expect(err.message).not.toMatch(/Unexpected Resolution/); - } - }); -}); diff --git a/src/core/public/http/abortable.ts b/src/core/public/http/abortable.ts deleted file mode 100644 index 86ebfbcf05ca25b..000000000000000 --- a/src/core/public/http/abortable.ts +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import { HttpHandler, HttpFetchOptions, AbortablePromise } from './types'; - -export function abortable(handler: HttpHandler) { - return (path: string, options: HttpFetchOptions = {}): AbortablePromise => { - const controller = options.signal - ? { signal: options.signal, abort: Function.prototype } - : new AbortController(); - const promise = handler(path, { ...options, signal: controller.signal }); - - // NOTE: We are only using Object.defineProperty with enumerable:false to be explicit about - // excluding promise.abort() which isn't the case with Object.assign. - return Object.defineProperty(promise, 'abort', { - enumerable: false, - value() { - controller.abort(); - }, - }); - }; -} diff --git a/src/core/public/http/fetch.ts b/src/core/public/http/fetch.ts new file mode 100644 index 000000000000000..9bfc13820cb5522 --- /dev/null +++ b/src/core/public/http/fetch.ts @@ -0,0 +1,91 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { merge } from 'lodash'; +import { format } from 'url'; + +import { HttpFetchOptions, HttpBody, Deps } from './types'; +import { HttpFetchError } from './http_fetch_error'; + +const JSON_CONTENT = /^(application\/(json|x-javascript)|text\/(x-)?javascript|x-json)(;.*)?$/; +const NDJSON_CONTENT = /^(application\/ndjson)(;.*)?$/; + +export const setup = ({ basePath, injectedMetadata }: Deps) => { + async function fetch(path: string, options: HttpFetchOptions = {}): Promise { + const { query, prependBasePath, ...fetchOptions } = merge( + { + method: 'GET', + credentials: 'same-origin', + prependBasePath: true, + headers: { + 'kbn-version': injectedMetadata.getKibanaVersion(), + 'Content-Type': 'application/json', + }, + }, + options + ); + const url = format({ + pathname: prependBasePath ? basePath.addToPath(path) : path, + query, + }); + + if ( + options.headers && + 'Content-Type' in options.headers && + options.headers['Content-Type'] === undefined + ) { + delete fetchOptions.headers['Content-Type']; + } + + let response; + let body = null; + + try { + response = await window.fetch(url, fetchOptions as RequestInit); + } catch (err) { + throw new HttpFetchError(err.message); + } + + const contentType = response.headers.get('Content-Type') || ''; + + try { + if (NDJSON_CONTENT.test(contentType)) { + body = await response.blob(); + } else if (JSON_CONTENT.test(contentType)) { + body = await response.json(); + } else { + body = await response.text(); + } + } catch (err) { + throw new HttpFetchError(err.message, response, body); + } + + if (!response.ok) { + throw new HttpFetchError(response.statusText, response, body); + } + + return body; + } + + function shorthand(method: string) { + return (path: string, options: HttpFetchOptions = {}) => fetch(path, { ...options, method }); + } + + return { fetch, shorthand }; +}; diff --git a/src/core/public/http/http_service.test.ts b/src/core/public/http/http_service.test.ts index 92e0223b367a597..51f4af44d313d3c 100644 --- a/src/core/public/http/http_service.test.ts +++ b/src/core/public/http/http_service.test.ts @@ -248,17 +248,6 @@ describe('http requests', async () => { expect(ndjson).toEqual(content); }); - - it('should return an abortable promise', () => { - const { http } = setupService(); - - fetchMock.get('*', {}); - - const abortable = http.fetch('/my/path'); - - expect(abortable).toBeInstanceOf(Promise); - expect(typeof abortable.abort).toBe('function'); - }); }); describe('addLoadingCount()', async () => { diff --git a/src/core/public/http/http_service.ts b/src/core/public/http/http_service.ts index b382ff865066e64..d00e71ecaab4c5b 100644 --- a/src/core/public/http/http_service.ts +++ b/src/core/public/http/http_service.ts @@ -28,101 +28,26 @@ import { tap, } from 'rxjs/operators'; -import { merge } from 'lodash'; -import { format } from 'url'; - -import { Deps, HttpFetchOptions, HttpBody } from './types'; -import { abortable } from './abortable'; -import { HttpFetchError } from './http_fetch_error'; - -const JSON_CONTENT = /^(application\/(json|x-javascript)|text\/(x-)?javascript|x-json)(;.*)?$/; -const NDJSON_CONTENT = /^(application\/ndjson)(;.*)?$/; +import { Deps } from './types'; +import { setup } from './fetch'; /** @internal */ export class HttpService { private readonly loadingCount$ = new Rx.BehaviorSubject(0); private readonly stop$ = new Rx.Subject(); - public setup({ basePath, injectedMetadata, fatalErrors }: Deps) { - async function fetch(path: string, options: HttpFetchOptions = {}): Promise { - const { query, prependBasePath, ...fetchOptions } = merge( - { - method: 'GET', - credentials: 'same-origin', - prependBasePath: true, - headers: { - 'kbn-version': injectedMetadata.getKibanaVersion(), - 'Content-Type': 'application/json', - }, - }, - options - ); - const url = format({ - pathname: prependBasePath ? basePath.addToPath(path) : path, - query, - }); - - if ( - options.headers && - 'Content-Type' in options.headers && - options.headers['Content-Type'] === undefined - ) { - delete fetchOptions.headers['Content-Type']; - } - - let response; - let body = null; - - try { - response = await window.fetch(url, fetchOptions as RequestInit); - } catch (err) { - throw new HttpFetchError(err.message); - } - - const contentType = response.headers.get('Content-Type') || ''; - - try { - if (NDJSON_CONTENT.test(contentType)) { - body = await response.blob(); - } else if (JSON_CONTENT.test(contentType)) { - body = await response.json(); - } else { - body = await response.text(); - } - } catch (err) { - throw new HttpFetchError(err.message, response, body); - } - - if (!response.ok) { - throw new HttpFetchError(response.statusText, response, body); - } - - return body; - } + public setup(deps: Deps) { + const { fetch, shorthand } = setup(deps); return { - fetch: abortable(fetch), - get: abortable((path: string, options: HttpFetchOptions = {}) => - fetch(path, { ...options, method: 'GET' }) - ), - head: abortable((path: string, options: HttpFetchOptions = {}) => - fetch(path, { ...options, method: 'HEAD' }) - ), - post: abortable((path: string, options: HttpFetchOptions = {}) => - fetch(path, { ...options, method: 'POST' }) - ), - put: abortable((path: string, options: HttpFetchOptions = {}) => - fetch(path, { ...options, method: 'PUT' }) - ), - patch: abortable((path: string, options: HttpFetchOptions = {}) => - fetch(path, { ...options, method: 'PATCH' }) - ), - delete: abortable((path: string, options: HttpFetchOptions = {}) => - fetch(path, { ...options, method: 'DELETE' }) - ), - options: abortable((path: string, options: HttpFetchOptions = {}) => - fetch(path, { ...options, method: 'OPTIONS' }) - ), + fetch, + delete: shorthand('HEAD'), + get: shorthand('GET'), + head: shorthand('HEAD'), + options: shorthand('OPTIONS'), + patch: shorthand('PATCH'), + post: shorthand('POST'), + put: shorthand('PUT'), addLoadingCount: (count$: Rx.Observable) => { count$ .pipe( @@ -149,7 +74,7 @@ export class HttpService { this.loadingCount$.next(this.loadingCount$.getValue() + delta); }, error: error => { - fatalErrors.add(error); + deps.fatalErrors.add(error); }, }); }, diff --git a/src/core/public/http/types.ts b/src/core/public/http/types.ts index 6a862c52b66f031..05f6ab502246ddb 100644 --- a/src/core/public/http/types.ts +++ b/src/core/public/http/types.ts @@ -52,9 +52,4 @@ export interface HttpFetchOptions extends HttpRequestInit { prependBasePath?: boolean; headers?: HttpHeadersInit; } -export interface Abortable { - abort: () => Promise; -} -export type AbortablePromise = Promise & Abortable; -export type HttpHandler = (path: string, options?: HttpFetchOptions) => Promise; export type HttpBody = BodyInit | null; diff --git a/src/legacy/ui/public/kfetch/kfetch.test.ts b/src/legacy/ui/public/kfetch/kfetch.test.ts index 71a0c53d56f073d..79bca9da1d273de 100644 --- a/src/legacy/ui/public/kfetch/kfetch.test.ts +++ b/src/legacy/ui/public/kfetch/kfetch.test.ts @@ -137,13 +137,6 @@ describe('kfetch', () => { } }); - it('should return an abortable promise', () => { - const abortable = kfetch({ pathname: '/my/path' }); - - expect(abortable).toBeInstanceOf(Promise); - expect(typeof abortable.abort).toBe('function'); - }); - describe('when throwing response error (KFetchError)', async () => { let error: KFetchError; beforeEach(async () => { diff --git a/src/legacy/ui/public/kfetch/kfetch.ts b/src/legacy/ui/public/kfetch/kfetch.ts index 0a5ba0aee489385..cb96e03eb132834 100644 --- a/src/legacy/ui/public/kfetch/kfetch.ts +++ b/src/legacy/ui/public/kfetch/kfetch.ts @@ -54,34 +54,15 @@ export function createKfetch(http: HttpSetup) { options: KFetchOptions, { prependBasePath = true }: KFetchKibanaOptions = {} ) { - const controller = options.signal - ? { signal: options.signal, abort: Function.prototype } - : new AbortController(); - const promise = new Promise((resolve, reject) => { - responseInterceptors( - requestInterceptors(withDefaultOptions(options)) - .then(({ pathname, query, ...restOptions }) => - http.fetch(pathname, { - ...restOptions, - signal: controller.signal, - query, - prependBasePath, - }) - ) - .catch(err => { - throw new KFetchError(err.response || { statusText: err.message }, err.body); - }) - ).then(resolve, reject); - }); - - // NOTE: We are only using Object.defineProperty with enumerable:false to be explicit about - // excluding promise.abort() which isn't the case with Object.assign. - return Object.defineProperty(promise, 'abort', { - enumerable: false, - value() { - controller.abort(); - }, - }); + return responseInterceptors( + requestInterceptors(withDefaultOptions(options)) + .then(({ pathname, ...restOptions }) => + http.fetch(pathname, { ...restOptions, prependBasePath }) + ) + .catch(err => { + throw new KFetchError(err.response || { statusText: err.message }, err.body); + }) + ); }; } diff --git a/x-pack/plugins/rollup/public/search/rollup_search_strategy.js b/x-pack/plugins/rollup/public/search/rollup_search_strategy.js index f996570e8bc4daf..3ae5e21a9b065bc 100644 --- a/x-pack/plugins/rollup/public/search/rollup_search_strategy.js +++ b/x-pack/plugins/rollup/public/search/rollup_search_strategy.js @@ -95,7 +95,9 @@ export const rollupSearchStrategy = { failedSearchRequests, } = await serializeAllFetchParams(allFetchParams, searchRequests); - const abortable = kfetch({ + const controller = new AbortController(); + const promise = kfetch({ + signal: controller.signal, pathname: '../api/rollup/search', method: 'POST', body: serializedFetchParams, @@ -103,7 +105,7 @@ export const rollupSearchStrategy = { return { searching: new Promise((resolve, reject) => { - abortable.then(result => { + promise.then(result => { resolve(shimHitsInFetchResponse(result)); }).catch(error => { const { @@ -123,7 +125,7 @@ export const rollupSearchStrategy = { reject(searchError); }); }), - abort: abortable.abort, + abort: () => controller.abort(), failedSearchRequests, }; },