Skip to content
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.

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

Send/receive error details with widgets #4492

Merged
merged 15 commits into from
Nov 9, 2024
6 changes: 5 additions & 1 deletion spec/unit/embedded.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,11 @@ class MockWidgetApi extends EventEmitter {
public getTurnServers = jest.fn(() => []);
public sendContentLoaded = jest.fn();

public transport = { reply: jest.fn() };
public transport = {
reply: jest.fn(),
send: jest.fn(),
sendComplete: jest.fn(),
};
}

declare module "../../src/types" {
Expand Down
28 changes: 28 additions & 0 deletions src/embedded.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import {
WidgetApi,
WidgetApiToWidgetAction,
WidgetApiResponseError,

Check failure on line 20 in src/embedded.ts

View workflow job for this annotation

GitHub Actions / Typescript Syntax Check

'"matrix-widget-api"' has no exported member named 'WidgetApiResponseError'. Did you mean 'IWidgetApiResponse'?
MatrixCapabilities,
IWidgetApiRequest,
IWidgetApiAcknowledgeResponseData,
Expand Down Expand Up @@ -45,6 +46,7 @@
} from "./client.ts";
import { SyncApi, SyncState } from "./sync.ts";
import { SlidingSyncSdk } from "./sliding-sync-sdk.ts";
import { MatrixError } from "./http-api/errors.ts";
import { User } from "./models/user.ts";
import { Room } from "./models/room.ts";
import { ToDeviceBatch, ToDevicePayload } from "./models/ToDeviceMessage.ts";
Expand Down Expand Up @@ -147,6 +149,26 @@
) {
super(opts);

const transportSend = this.widgetApi.transport.send.bind(this.widgetApi.transport);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this bind?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is used as a way to preserve the original send method so it can still be used by the function that replaces it.

Alternatively this could have been an arrow function, but that would require specifying a parameter list. Using bind gets around that.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a really nasty pattern, this looks like somewhere a subclass makes far more sense rather than overwriting class methods, what's the rationale for not doing it in a more conventional way?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because the send methods don't belong to WidgetApi, but to ITransport, meaning that subclassing (or even a composition-based approach) would have to work harder to get to them.

Any non-bind approach I can think of ends up being more complicated / requires much more copied code than this.

// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
AndrewFerr marked this conversation as resolved.
Show resolved Hide resolved
this.widgetApi.transport.send = async (action, data) => {
try {
return await transportSend(action, data);
} catch (error) {
processAndThrow(error);
}
};

const transportSendComplete = this.widgetApi.transport.sendComplete.bind(this.widgetApi.transport);
// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
AndrewFerr marked this conversation as resolved.
Show resolved Hide resolved
this.widgetApi.transport.sendComplete = async (action, data) => {
try {
return await transportSendComplete(action, data);
} catch (error) {
processAndThrow(error);
}
};

this.widgetApiReady = new Promise<void>((resolve) => this.widgetApi.once("ready", resolve));

// Request capabilities for the functionality this client needs to support
Expand Down Expand Up @@ -523,3 +545,9 @@
}
}
}

function processAndThrow(error: unknown): never {
throw error instanceof WidgetApiResponseError && error.data.matrix_api_error

Check failure on line 550 in src/embedded.ts

View workflow job for this annotation

GitHub Actions / Typescript Syntax Check

'error' is of type 'unknown'.
? MatrixError.fromWidgetApiErrorData(error.data.matrix_api_error)

Check failure on line 551 in src/embedded.ts

View workflow job for this annotation

GitHub Actions / Typescript Syntax Check

'error' is of type 'unknown'.
: error;
AndrewFerr marked this conversation as resolved.
Show resolved Hide resolved
}
29 changes: 29 additions & 0 deletions src/http-api/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
limitations under the License.
*/

import { IMatrixApiError as IWidgetMatrixError } from "matrix-widget-api";

Check failure on line 17 in src/http-api/errors.ts

View workflow job for this annotation

GitHub Actions / Typescript Syntax Check

Module '"matrix-widget-api"' has no exported member 'IMatrixApiError'.

import { IUsageLimit } from "../@types/partials.ts";
import { MatrixEvent } from "../models/event.ts";

Expand Down Expand Up @@ -131,6 +133,33 @@
}
return null;
}

/**
* @returns this error expressed as a {@link IWidgetMatrixError}
* for use by Widget API error responses.
*/
public asWidgetApiErrorData(): IWidgetMatrixError {
const headers: Record<string, string> = {};
if (this.httpHeaders) {
for (const [name, value] of this.httpHeaders) {
headers[name] = value;
}
}
return {
http_status: this.httpStatus ?? 400,
http_headers: headers,
url: this.url ?? "",
response: {
errcode: this.errcode ?? "M_UNKNOWN",
error: this.name,
...this.data,
},
};
}

public static fromWidgetApiErrorData(data: IWidgetMatrixError): MatrixError {
return new MatrixError(data.response, data.http_status, data.url, undefined, new Headers(data.http_headers));
}
}

/**
Expand Down
Loading