From bbb579a10bdd8cfcd6519d6bee76eefe86a2673e Mon Sep 17 00:00:00 2001 From: Felix Becker Date: Thu, 19 Sep 2019 22:04:05 +0200 Subject: [PATCH] Use fromFetch() instead of ajax() (#5561) --- .eslintrc.js | 2 +- browser/src/extension/scripts/background.tsx | 6 +-- browser/src/libs/bitbucket/api.ts | 11 +++-- browser/src/libs/gitlab/api.ts | 11 +++-- browser/src/libs/phabricator/backend.tsx | 12 +++-- browser/src/platform/context.ts | 7 +-- browser/src/platform/worker.ts | 15 +++--- .../web/src/CriticalConfigEditor.tsx | 46 +++++++++---------- .../api/client/services/extensionsService.ts | 14 +++--- shared/src/backend/fetch.ts | 12 +++++ shared/src/graphql/graphql.ts | 27 ++++------- shared/src/util/errors.ts | 19 -------- web/src/backend/backend.ts | 18 +++----- 13 files changed, 89 insertions(+), 111 deletions(-) create mode 100644 shared/src/backend/fetch.ts diff --git a/.eslintrc.js b/.eslintrc.js index 9292dc8d5e2..e312841bcf2 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -33,7 +33,7 @@ const config = { rules: { // Rules that are specific to this repo // All other rules should go into https://github.com/sourcegraph/eslint-config - 'no-restricted-imports': ['error', { paths: ['highlight.js', 'marked'] }], + 'no-restricted-imports': ['error', { paths: ['highlight.js', 'marked', 'rxjs/ajax'] }], 'react/forbid-elements': [ 'error', { diff --git a/browser/src/extension/scripts/background.tsx b/browser/src/extension/scripts/background.tsx index 83ce980151e..89e962a6198 100644 --- a/browser/src/extension/scripts/background.tsx +++ b/browser/src/extension/scripts/background.tsx @@ -62,11 +62,7 @@ const requestGraphQL = ({ variables, baseUrl, headers: getHeaders(), - requestOptions: { - crossDomain: true, - withCredentials: true, - async: true, - }, + credentials: 'include', }) ) ) diff --git a/browser/src/libs/bitbucket/api.ts b/browser/src/libs/bitbucket/api.ts index 5f4417b84fa..a295db7bdad 100644 --- a/browser/src/libs/bitbucket/api.ts +++ b/browser/src/libs/bitbucket/api.ts @@ -1,11 +1,12 @@ import { first } from 'lodash' import { Observable } from 'rxjs' -import { ajax } from 'rxjs/ajax' -import { filter, map } from 'rxjs/operators' +import { filter, map, switchMap } from 'rxjs/operators' import { memoizeObservable } from '../../../../shared/src/util/memoizeObservable' import { isDefined } from '../../../../shared/src/util/types' import { DiffResolvedRevSpec } from '../../shared/repo' import { BitbucketRepoInfo } from './scrape' +import { fromFetch } from 'rxjs/fetch' +import { checkOk } from '../../../../shared/src/backend/fetch' // // PR API /rest/api/1.0/projects/SG/repos/go-langserver/pull-requests/1 @@ -24,7 +25,11 @@ const buildURL = (project: string, repoSlug: string, path: string): string => project )}/repos/${repoSlug}${path}` -const get = (url: string): Observable => ajax.get(url).pipe(map(({ response }) => response as T)) +const get = (url: string): Observable => + fromFetch(url).pipe( + map(checkOk), + switchMap(response => response.json()) + ) interface Repo { project: { key: string } diff --git a/browser/src/libs/gitlab/api.ts b/browser/src/libs/gitlab/api.ts index ff30c7f9f86..f0e2953f433 100644 --- a/browser/src/libs/gitlab/api.ts +++ b/browser/src/libs/gitlab/api.ts @@ -1,10 +1,11 @@ import { first } from 'lodash' import { Observable } from 'rxjs' -import { ajax } from 'rxjs/ajax' -import { map } from 'rxjs/operators' +import { map, switchMap } from 'rxjs/operators' import { memoizeObservable } from '../../../../shared/src/util/memoizeObservable' import { GitLabDiffInfo } from './scrape' +import { fromFetch } from 'rxjs/fetch' +import { checkOk } from '../../../../shared/src/backend/fetch' /** * Significant revisions for a merge request. @@ -36,7 +37,11 @@ type GetBaseCommitIDInput = Pick `${window.location.origin}/api/v4/projects/${encodeURIComponent(owner)}%2f${projectName}${path}` -const get = (url: string): Observable => ajax.get(url).pipe(map(({ response }) => response as T)) +const get = (url: string): Observable => + fromFetch(url).pipe( + map(checkOk), + switchMap(response => response.json()) + ) /** * Get the base commit ID for a merge request. diff --git a/browser/src/libs/phabricator/backend.tsx b/browser/src/libs/phabricator/backend.tsx index 20819c77d9d..bc480d34392 100644 --- a/browser/src/libs/phabricator/backend.tsx +++ b/browser/src/libs/phabricator/backend.tsx @@ -8,10 +8,11 @@ import { storage } from '../../browser/storage' import { isExtension } from '../../context' import { resolveRepo } from '../../shared/repo/backend' import { normalizeRepoName } from './util' -import { ajax } from 'rxjs/ajax' +import { fromFetch } from 'rxjs/fetch' import { EREPONOTFOUND } from '../../../../shared/src/backend/errors' import { RepoSpec, FileSpec, ResolvedRevSpec } from '../../../../shared/src/util/url' import { RevisionSpec, DiffSpec, BaseDiffSpec } from '.' +import { checkOk } from '../../../../shared/src/backend/fetch' interface PhabEntity { id: string // e.g. "48" @@ -127,16 +128,17 @@ export function queryConduitHelper(endpoint: string, params: {}): Observable< for (const [key, value] of Object.entries(params)) { form.set(`params[${key}]`, JSON.stringify(value)) } - return ajax({ - url: window.location.origin + endpoint, + return fromFetch(window.location.origin + endpoint, { method: 'POST', body: form, - withCredentials: true, + credentials: 'include', headers: { Accept: 'application/json', }, }).pipe( - map(({ response }: { response: ConduitResponse }) => { + map(checkOk), + switchMap(response => response.json()), + map((response: ConduitResponse) => { if (response.error_code !== null) { throw new Error(`error ${response.error_code}: ${response.error_info}`) } diff --git a/browser/src/platform/context.ts b/browser/src/platform/context.ts index cd090e153a9..86ca387784a 100644 --- a/browser/src/platform/context.ts +++ b/browser/src/platform/context.ts @@ -70,12 +70,7 @@ export function createPlatformContext( request, variables, baseUrl: window.SOURCEGRAPH_URL, - headers: {}, - requestOptions: { - crossDomain: true, - withCredentials: true, - async: true, - }, + credentials: 'include', }) }) ) diff --git a/browser/src/platform/worker.ts b/browser/src/platform/worker.ts index 835687e59d3..9b95e6c6953 100644 --- a/browser/src/platform/worker.ts +++ b/browser/src/platform/worker.ts @@ -1,13 +1,12 @@ -import { ajax } from 'rxjs/ajax' import { DEFAULT_SOURCEGRAPH_URL } from '../shared/util/context' +import { checkOk } from '../../../shared/src/backend/fetch' export async function createBlobURLForBundle(bundleURL: string): Promise { - const req = await ajax({ - url: bundleURL, + const response = await fetch(bundleURL, { // Include credentials when fetching extensions from the private registry - withCredentials: new URL(bundleURL).origin !== DEFAULT_SOURCEGRAPH_URL, - crossDomain: true, - responseType: 'blob', - }).toPromise() - return window.URL.createObjectURL(req.response) + credentials: new URL(bundleURL).origin !== DEFAULT_SOURCEGRAPH_URL ? 'include' : 'omit', + }) + checkOk(response) + const blob = await response.blob() + return window.URL.createObjectURL(blob) } diff --git a/cmd/management-console/web/src/CriticalConfigEditor.tsx b/cmd/management-console/web/src/CriticalConfigEditor.tsx index ab43706a729..addeb78b97d 100644 --- a/cmd/management-console/web/src/CriticalConfigEditor.tsx +++ b/cmd/management-console/web/src/CriticalConfigEditor.tsx @@ -4,14 +4,12 @@ import * as _monaco from 'monaco-editor' import { truncate } from 'lodash' import * as React from 'react' -import { from, interval, Observable, of, Subject, Subscription, timer } from 'rxjs' -import { ajax } from 'rxjs/ajax' -import { catchError, delay, distinctUntilChanged, mapTo, startWith, takeUntil } from 'rxjs/operators' +import { from, Subscription, timer } from 'rxjs' +import { fromFetch } from 'rxjs/fetch' +import { switchMap } from 'rxjs/operators' import './CriticalConfigEditor.scss' import { MonacoEditor } from './MonacoEditor' -const DEBUG_LOADING_STATE_DELAY = 0 // ms - /** * Amount of time to wait before showing the loading indicator. */ @@ -294,12 +292,9 @@ export class CriticalConfigEditor extends React.PureComponent { private configEditor?: _monaco.editor.ICodeEditor - private componentUpdates = new Subject() private subscriptions = new Subscription() public componentDidMount(): void { - const componentUpdates = this.componentUpdates.pipe(startWith(this.props)) - // Periodically rerender our component in case our request takes longer // than `WAIT_BEFORE_SHOWING_LOADER` and we need to show the loading // indicator. @@ -307,24 +302,27 @@ export class CriticalConfigEditor extends React.PureComponent { // Load the initial critical config. this.subscriptions.add( - ajax('/api/get') + fromFetch('/api/get') .pipe( - delay(DEBUG_LOADING_STATE_DELAY), - catchError(err => of(err.xhr)) - ) - .subscribe(resp => { - if (resp.status !== 200) { - const msg = 'error saving: ' + resp.status - console.error(msg) - alert(msg) // TODO(slimsag): Better general error state here. - return - } - - const config = resp.response as Configuration - this.setState({ - criticalConfig: config, - content: config.Contents, + switchMap(response => { + if (response.status !== 200) { + throw new Error(`Error saving: ${response.status} ${response.statusText}`) + } + return response.json() }) + ) + .subscribe({ + next: (config: Configuration) => { + this.setState({ + criticalConfig: config, + content: config.Contents, + }) + }, + error: error => { + console.error(error) + alert(error.message) // TODO(slimsag): Better general error state here. + return + }, }) ) } diff --git a/shared/src/api/client/services/extensionsService.ts b/shared/src/api/client/services/extensionsService.ts index c612bf4d1ad..ade6a258677 100644 --- a/shared/src/api/client/services/extensionsService.ts +++ b/shared/src/api/client/services/extensionsService.ts @@ -1,6 +1,5 @@ import { isEqual } from 'lodash' import { combineLatest, from, Observable, ObservableInput, of, Subscribable } from 'rxjs' -import { ajax } from 'rxjs/ajax' import { catchError, distinctUntilChanged, map, switchMap, tap } from 'rxjs/operators' import { ConfiguredExtension, @@ -15,6 +14,8 @@ import { combineLatestOrDefault } from '../../../util/rxjs/combineLatestOrDefaul import { isDefined } from '../../../util/types' import { SettingsService } from './settings' import { ModelService } from './modelService' +import { fromFetch } from 'rxjs/fetch' +import { checkOk } from '../../../backend/fetch' /** * The information about an extension necessary to execute and activate it. @@ -25,14 +26,11 @@ export interface ExecutableExtension extends Pick => - ajax({ - url: `${baseUrl}/package.json`, - responseType: 'json', - crossDomain: true, - async: true, - }).pipe( + fromFetch(`${baseUrl}/package.json`).pipe( + map(checkOk), + switchMap(response => response.json()), map( - ({ response }): ConfiguredExtension => ({ + (response): ConfiguredExtension => ({ id: response.name, manifest: { url: `${baseUrl}/${response.main.replace('dist/', '')}`, diff --git a/shared/src/backend/fetch.ts b/shared/src/backend/fetch.ts new file mode 100644 index 00000000000..d4833638f0e --- /dev/null +++ b/shared/src/backend/fetch.ts @@ -0,0 +1,12 @@ +/** + * Checks if a given fetch Response has a HTTP 2xx status code and throws an Error otherwise. + */ +export function checkOk(response: Response): Response { + if (!response.ok) { + throw Object.assign( + new Error(`Request to ${response.url} failed with ${response.status} ${response.statusText}`), + { response } + ) + } + return response +} diff --git a/shared/src/graphql/graphql.ts b/shared/src/graphql/graphql.ts index 653bc8142d0..4dd2fb46759 100644 --- a/shared/src/graphql/graphql.ts +++ b/shared/src/graphql/graphql.ts @@ -1,9 +1,10 @@ import { Observable } from 'rxjs' -import { ajax, AjaxRequest, AjaxResponse } from 'rxjs/ajax' -import { catchError, map } from 'rxjs/operators' +import { map, switchMap } from 'rxjs/operators' import { Omit } from 'utility-types' -import { createAggregateError, normalizeAjaxError } from '../util/errors' +import { createAggregateError } from '../util/errors' +import { checkOk } from '../backend/fetch' import * as GQL from './schema' +import { fromFetch } from 'rxjs/fetch' /** * Use this template string tag for all GraphQL queries. @@ -50,34 +51,26 @@ export const createInvalidGraphQLMutationResponseError = (queryName: string): Gr queryName, }) -export interface GraphQLRequestOptions { - headers: AjaxRequest['headers'] - requestOptions?: Partial> +export interface GraphQLRequestOptions extends Omit { baseUrl?: string } export function requestGraphQL({ request, variables = {}, - headers, - requestOptions = {}, baseUrl = '', + ...options }: GraphQLRequestOptions & { request: string variables?: {} }): Observable> { const nameMatch = request.match(/^\s*(?:query|mutation)\s+(\w+)/) - return ajax({ + return fromFetch(`${baseUrl}/.api/graphql${nameMatch ? '?' + nameMatch[1] : ''}`, { + ...options, method: 'POST', - url: `${baseUrl}/.api/graphql${nameMatch ? '?' + nameMatch[1] : ''}`, - headers, body: JSON.stringify({ query: request, variables }), - ...requestOptions, }).pipe( - catchError(err => { - normalizeAjaxError(err) - throw err - }), - map(({ response }) => response) + map(checkOk), + switchMap(response => response.json()) ) } diff --git a/shared/src/util/errors.ts b/shared/src/util/errors.ts index 8dae182ae84..1a897e7e2c8 100644 --- a/shared/src/util/errors.ts +++ b/shared/src/util/errors.ts @@ -43,22 +43,3 @@ export const createAggregateError = (errors: ErrorLike[] = []): AggregateError = name: 'AggregateError' as const, errors: errors.map(asError), }) - -/** - * Improves error messages in case of ajax errors - */ -export const normalizeAjaxError = (err: any): void => { - if (!err) { - return - } - if (typeof err.status === 'number') { - if (err.status === 0) { - err.message = 'Unable to reach server. Check your network connection and try again in a moment.' - } else { - err.message = `Unexpected HTTP error: ${err.status}` - if (err.xhr && err.xhr.statusText) { - err.message += ` ${err.xhr.statusText}` - } - } - } -} diff --git a/web/src/backend/backend.ts b/web/src/backend/backend.ts index 0b2cc76ace1..7a6aef59a0f 100644 --- a/web/src/backend/backend.ts +++ b/web/src/backend/backend.ts @@ -1,17 +1,11 @@ -import { ajax, AjaxResponse } from 'rxjs/ajax' import { Observable } from 'rxjs' -import { catchError, map } from 'rxjs/operators' -import { normalizeAjaxError } from '../../../shared/src/util/errors' +import { map, switchMap } from 'rxjs/operators' +import { fromFetch } from 'rxjs/fetch' +import { checkOk } from '../../../shared/src/backend/fetch' export function backendRequest(url: string): Observable { - return ajax({ - url, - headers: window.context.xhrHeaders, - }).pipe( - catchError(err => { - normalizeAjaxError(err) - throw err - }), - map(({ response }) => response) + return fromFetch(url, { headers: window.context.xhrHeaders }).pipe( + map(checkOk), + switchMap(response => response.json()) ) }