Skip to content

WIP: Fetch support #2166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

WIP: Fetch support #2166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d51e54d
Fetch support
bradjones1 Feb 8, 2022
c0e5644
A bit more hacking
bradjones1 Feb 9, 2022
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
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 @@ -55,11 +55,11 @@
"browser-request": "^0.3.3",
"bs58": "^4.0.1",
"content-type": "^1.0.4",
"cross-fetch": "^3.1.5",
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth investigating but this package uses whatwg-fetch (browser polyfill) which may create downstream issues in Element Web inflating bundle size with a polyfill which we never care about as all compatible browsers already support fetch.

"loglevel": "^1.7.1",
"matrix-events-sdk": "^0.0.1-beta.6",
"p-retry": "^4.5.0",
"qs": "^6.9.6",
"request": "^2.88.2",
"qs": "^6.10.3",
"unhomoglyph": "^1.0.6"
},
"devDependencies": {
Expand All @@ -80,7 +80,6 @@
"@types/content-type": "^1.1.5",
"@types/jest": "^26.0.20",
"@types/node": "12",
"@types/request": "^2.48.5",
"@typescript-eslint/eslint-plugin": "^5.6.0",
"@typescript-eslint/parser": "^5.6.0",
"allchange": "^1.0.6",
Expand Down
14 changes: 0 additions & 14 deletions src/browser-index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,8 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import request from "browser-request";
import queryString from "qs";

import * as matrixcs from "./matrix";

matrixcs.request(function(opts, fn) {
// We manually fix the query string for browser-request because
// it doesn't correctly handle cases like ?via=one&via=two. Instead
// we mimic `request`'s query string interface to make it all work
// as expected.
// browser-request will happily take the constructed string as the
// query string without trying to modify it further.
opts.qs = queryString.stringify(opts.qs || {}, opts.qsStringifyOptions);
return request(opts, fn);
});

// just *accessing* indexedDB throws an exception in firefox with
// indexeddb disabled.
let indexedDB;
Expand Down
12 changes: 6 additions & 6 deletions src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ import { IThreepid } from "./@types/threepids";
import { CryptoStore } from "./crypto/store/base";
import { MediaHandler } from "./webrtc/mediaHandler";

import fetch from 'cross-fetch';

export type Store = IStore;
export type SessionStore = WebStorageSessionStore;

Expand Down Expand Up @@ -212,12 +214,10 @@ export interface ICreateClientOpts {
scheduler?: MatrixScheduler;

/**
* The function to invoke for HTTP
* requests. The value of this property is typically <code>require("request")
* </code> as it returns a function which meets the required interface. See
* {@link requestFunction} for more information.
* The function to invoke for HTTP requests.
* See {@link fetch} for more information.
*/
request?: IHttpOpts["request"];
fetch?: typeof fetch;
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe instead of depending on cross-fetch we could just have the caller pass their favourite fetch, as for typing a shim in /@types/ would be fine


userId?: string;

Expand Down Expand Up @@ -845,7 +845,7 @@ export class MatrixClient extends EventEmitter {
baseUrl: opts.baseUrl,
idBaseUrl: opts.idBaseUrl,
accessToken: opts.accessToken,
request: opts.request,
fetch: opts.fetch,
prefix: PREFIX_R0,
onlyData: true,
extraParams: opts.queryParams,
Expand Down
151 changes: 51 additions & 100 deletions src/http-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import { parse as parseContentType, ParsedMediaType } from "content-type";
import EventEmitter from "events";

import type { IncomingHttpHeaders, IncomingMessage } from "http";
import type { Request as _Request, CoreOptions } from "request";
// we use our own implementation of setTimeout, so that if we get suspended in
// the middle of a /sync, we cancel the sync as soon as we awake, rather than
// waiting for the delay to elapse.
Expand All @@ -35,6 +34,7 @@ import { IDeferred } from "./utils";
import { Callback } from "./client";
import * as utils from "./utils";
import { logger } from './logger';
const queryString = require('qs');
Copy link
Member

Choose a reason for hiding this comment

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

imports not requires please


/*
TODO:
Expand Down Expand Up @@ -73,16 +73,6 @@ export const PREFIX_IDENTITY_V2 = "/_matrix/identity/v2";
*/
export const PREFIX_MEDIA_R0 = "/_matrix/media/r0";

type RequestProps = "method"
| "withCredentials"
| "json"
| "headers"
| "qs"
| "body"
| "qsStringifyOptions"
| "useQuerystring"
| "timeout";

export interface IHttpOpts {
baseUrl: string;
idBaseUrl?: string;
Expand All @@ -92,24 +82,12 @@ export interface IHttpOpts {
extraParams?: Record<string, string>;
localTimeoutMs?: number;
useAuthorizationHeader?: boolean;
request(opts: Pick<CoreOptions, RequestProps> & {
uri: string;
method: Method;
// eslint-disable-next-line camelcase
_matrix_opts: IHttpOpts;
}, callback: RequestCallback): IRequest;
}

interface IRequest extends _Request {
onprogress?(e: unknown): void;
fetch: typeof fetch;
}

interface IRequestOpts<T> {
prefix?: string;
localTimeoutMs?: number;
headers?: Record<string, string>;
json?: boolean; // defaults to true
qsStringifyOptions?: CoreOptions["qsStringifyOptions"];
bodyParser?(body: string): T;
}

Expand Down Expand Up @@ -333,7 +311,7 @@ export class MatrixHttpApi {
// (browser-request doesn't support progress either, which is also kind
// of important here)

const upload = { loaded: 0, total: 0 } as IUpload;
const upload = {loaded: 0, total: 0} as IUpload;
let promise: IAbortablePromise<UploadContentResponseType<O>>;

// XMLHttpRequest doesn't parse JSON for us. request normally does, but
Expand All @@ -342,7 +320,7 @@ export class MatrixHttpApi {
// way, we have to JSON-parse the response ourselves.
let bodyParser = null;
if (!rawResponse) {
bodyParser = function(rawBody: string) {
bodyParser = function (rawBody: string) {
let body = JSON.parse(rawBody);
if (onlyContentUri) {
body = body.content_uri;
Expand All @@ -359,15 +337,15 @@ export class MatrixHttpApi {
const xhr = new global.XMLHttpRequest();
const cb = requestCallback(defer, opts.callback, this.opts.onlyData);

const timeoutFn = function() {
const timeoutFn = function () {
xhr.abort();
cb(new Error('Timeout'));
};

// set an initial timeout of 30s; we'll advance it each time we get a progress notification
let timeoutTimer = callbacks.setTimeout(timeoutFn, 30000);

xhr.onreadystatechange = function() {
xhr.onreadystatechange = function () {
let resp: string;
switch (xhr.readyState) {
case global.XMLHttpRequest.DONE:
Expand All @@ -392,7 +370,7 @@ export class MatrixHttpApi {
break;
}
};
xhr.upload.addEventListener("progress", function(ev) {
xhr.upload.addEventListener("progress", function (ev) {
callbacks.clearTimeout(timeoutTimer);
upload.loaded = ev.loaded;
upload.total = ev.total;
Expand Down Expand Up @@ -440,7 +418,7 @@ export class MatrixHttpApi {
promise = this.authedRequest(
opts.callback, Method.Post, "/upload", queryParams, body, {
prefix: "/_matrix/media/r0",
headers: { "Content-Type": contentType },
headers: {"Content-Type": contentType},
json: false,
bodyParser,
},
Expand Down Expand Up @@ -497,26 +475,21 @@ export class MatrixHttpApi {
}

const opts = {
uri: fullUri,
method,
withCredentials: false,
json: true, // we want a JSON response if we can
_matrix_opts: this.opts,
headers: {},
} as Parameters<IHttpOpts["request"]>[0];

if (method === Method.Get) {
opts.qs = params;
} else if (typeof params === "object") {
opts.json = params;
}

if (accessToken) {
opts.headers['Authorization'] = `Bearer ${accessToken}`;
}
headers: accessToken
? {Authorization: `Bearer ${accessToken}`}
: {},
body: (method !== Method.Get)
? JSON.stringify(params)
: null,
} as RequestInfo;

const defer = utils.defer<T>();
this.opts.request(opts, requestCallback(defer, callback, this.opts.onlyData));
const url = (method === Method.Get)
? fullUri + queryString.stringify(params)
: fullUri;
this.requestOtherUrl(callback, method, url, null, null, opts);
return defer.promise;
}

Expand Down Expand Up @@ -782,7 +755,7 @@ export class MatrixHttpApi {
}

if (bodyParser === undefined) {
bodyParser = function(rawBody: string) {
bodyParser = function (rawBody: string) {
return JSON.parse(rawBody);
};
}
Expand All @@ -792,17 +765,15 @@ export class MatrixHttpApi {

let timeoutId: number;
let timedOut = false;
let req: IRequest;
const localTimeoutMs = opts.localTimeoutMs || this.opts.localTimeoutMs;

const resetTimeout = () => {
if (localTimeoutMs) {
if (timeoutId) {
callbacks.clearTimeout(timeoutId);
}
timeoutId = callbacks.setTimeout(function() {
timeoutId = callbacks.setTimeout(function () {
timedOut = true;
req?.abort?.();
defer.reject(new MatrixError({
error: "Locally timed out waiting for a response",
errcode: "ORG.MATRIX.JSSDK_TIMEOUT",
Expand All @@ -815,58 +786,38 @@ export class MatrixHttpApi {

const reqPromise = defer.promise as IAbortablePromise<ResponseType<T, O>>;

try {
req = this.opts.request(
{
uri: uri,
method: method,
withCredentials: false,
qs: queryParams,
qsStringifyOptions: opts.qsStringifyOptions,
useQuerystring: true,
body: data,
json: false,
timeout: localTimeoutMs,
headers: headers || {},
_matrix_opts: this.opts,
},
(err, response, body) => {
if (localTimeoutMs) {
callbacks.clearTimeout(timeoutId);
if (timedOut) {
return; // already rejected promise
}
}

const handlerFn = requestCallback(defer, callback, this.opts.onlyData, bodyParser);
handlerFn(err, response, body);
},
);
if (req) {
// This will only work in a browser, where opts.request is the
// `browser-request` import. Currently, `request` does not support progress
// updates - see https://github.com/request/request/pull/2346.
// `browser-request` returns an XHRHttpRequest which exposes `onprogress`
if ('onprogress' in req) {
req.onprogress = (e) => {
// Prevent the timeout from rejecting the deferred promise if progress is
// seen with the request
resetTimeout();
};
}

// FIXME: This is EVIL, but I can't think of a better way to expose
// abort() operations on underlying HTTP requests :(
if (req.abort) {
reqPromise.abort = req.abort.bind(req);
const qs = queryString.stringify(queryParams || {}, opts.qsStringifyOptions);
let res;
const controller = new AbortController();
const signal = controller.signal;
this.opts.fetch(qs ? `${uri}?${qs}` : uri, {
method,
headers: headers || [],
body: data,
mode: "cors",
credentials: "omit",
signal,
}).then((_res) => {
res = _res;
if (localTimeoutMs) {
callbacks.clearTimeout(timeoutId);
if (timedOut) {
return; // already rejected promise
}
}
} catch (ex) {
defer.reject(ex);
if (callback) {
callback(ex);
}
}
return res.json();
}).then((body) => {
const handlerFn = requestCallback(defer, callback, this.opts.onlyData, bodyParser);
handlerFn(null, res, body.toString());
}).catch((ex) => {
const handlerFn = requestCallback(
defer, callback, this.opts.onlyData,
);
handlerFn(ex, null, null);
});
reqPromise.abort = () => {
controller.abort();
};
return reqPromise;
}
}
Expand Down
5 changes: 0 additions & 5 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,10 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import request from "request";

import * as matrixcs from "./matrix";
import * as utils from "./utils";
import { logger } from './logger';

matrixcs.request(request);

try {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const crypto = require('crypto');
Expand All @@ -32,4 +28,3 @@ try {

export * from "./matrix";
export default matrixcs;

Loading