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.

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

Jump to bottom

Make the js-sdk conform to tsc --strict #2835

Merged
merged 6 commits into from
Nov 3, 2022
Merged

Make the js-sdk conform to tsc --strict #2835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a1eee8
Make the js-sdk conform to tsc --strict
t3chguy Nov 1, 2022
1dcca06
Update tsconfig.json
t3chguy Nov 2, 2022
f729f09
Patch matrix-events-sdk until https://github.com/matrix-org/matrix-ev…
t3chguy Nov 3, 2022
2746dc3
Merge remote-tracking branch 'origin/t3chguy/tsc-strict4' into t3chgu…
t3chguy Nov 3, 2022
6de7c10
Merge branch 'develop' of github.com:matrix-org/matrix-js-sdk into t3…
t3chguy Nov 3, 2022
f1405e7
Fix tests
t3chguy Nov 3, 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: 4 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
},
Copy link
Contributor

@MadLittleMods MadLittleMods Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like there are a lot of places where we're using ! without any reason to be sure that values are actually defined and non-null; I'm not clear on why this is an improvement from a type-safety standpoint?

-- @duxovni, #2835 (review)

What's the summary/thoughts here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The upshot from the individual comments was basically, we've got a lot of messy stuff with overloads where it'd be a pain to convince typescript that we're doing the right thing, so it's best to just tell typescript it's fine so we can go ahead and enable strict mode globally which will make life easier in general.

Copy link
Contributor

Choose a reason for hiding this comment

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

And I assume we should avoid this situation more in future code if possible?

Do our current overloads doom us to ! usage forever? Is there certain overload patterns we should avoid in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Overloads with different combinations of optionals, e.g. A is optional & B is mandatory | A is mandatory & B is optional will either need an if-case per combination to handle or !s

"scripts": {
"prepublishOnly": "yarn build",
"postinstall": "patch-package",
"start": "echo THIS IS FOR LEGACY PURPOSES ONLY. && babel src -w -s -d lib --verbose --extensions \".ts,.js\"",
"dist": "echo 'This is for the release script so it can make assets (browser bundle).' && yarn build",
"clean": "rimraf lib dist",
Expand Down Expand Up @@ -58,7 +59,7 @@
"bs58": "^5.0.0",
"content-type": "^1.0.4",
"loglevel": "^1.7.1",
"matrix-events-sdk": "^0.0.1-beta.7",
"matrix-events-sdk": "0.0.1-beta.7",
"p-retry": "4",
"qs": "^6.9.6",
"unhomoglyph": "^1.0.6"
Expand Down Expand Up @@ -105,6 +106,8 @@
"jest-sonar-reporter": "^2.0.0",
"jsdoc": "^3.6.6",
"matrix-mock-request": "^2.5.0",
"patch-package": "^6.5.0",
"postinstall-postinstall": "^2.1.0",
"rimraf": "^3.0.2",
"terser": "^5.5.1",
"tsify": "^5.0.2",
Expand Down
12 changes: 12 additions & 0 deletions patches/matrix-events-sdk+0.0.1-beta.7.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
diff --git a/node_modules/matrix-events-sdk/lib/NamespacedMap.d.ts b/node_modules/matrix-events-sdk/lib/NamespacedMap.d.ts
index c141b11..461f528 100644
--- a/node_modules/matrix-events-sdk/lib/NamespacedMap.d.ts
+++ b/node_modules/matrix-events-sdk/lib/NamespacedMap.d.ts
@@ -1,6 +1,6 @@
import { NamespacedValue } from "./NamespacedValue";
import { Optional } from "./types";
-declare type NS = NamespacedValue<Optional<string>, Optional<string>>;
+declare type NS = NamespacedValue<string, string>;
/**
* A `Map` implementation which accepts a NamespacedValue as a key, and arbitrary value. The
* namespaced value must be a string type.
8 changes: 2 additions & 6 deletions spec/integ/matrix-client-syncing.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,15 +707,11 @@ describe("MatrixClient syncing", () => {
awaitSyncEvent(2),
]).then(() => {
const room = client!.getRoom(roomOne)!;
const stateAtStart = room.getLiveTimeline().getState(
EventTimeline.BACKWARDS,
);
const stateAtStart = room.getLiveTimeline().getState(EventTimeline.BACKWARDS)!;
const startRoomNameEvent = stateAtStart.getStateEvents('m.room.name', '');
expect(startRoomNameEvent.getContent().name).toEqual('Old room name');

const stateAtEnd = room.getLiveTimeline().getState(
EventTimeline.FORWARDS,
);
const stateAtEnd = room.getLiveTimeline().getState(EventTimeline.FORWARDS)!;
const endRoomNameEvent = stateAtEnd.getStateEvents('m.room.name', '');
expect(endRoomNameEvent.getContent().name).toEqual('A new room name');
});
Expand Down
24 changes: 12 additions & 12 deletions spec/unit/event-timeline.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ describe("EventTimeline", function() {
];
timeline.initialiseState(events);
// @ts-ignore private prop
const timelineStartState = timeline.startState;
const timelineStartState = timeline.startState!;
expect(mocked(timelineStartState).setStateEvents).toHaveBeenCalledWith(
events,
{ timelineWasEmpty: undefined },
);
// @ts-ignore private prop
const timelineEndState = timeline.endState;
const timelineEndState = timeline.endState!;
expect(mocked(timelineEndState).setStateEvents).toHaveBeenCalledWith(
events,
{ timelineWasEmpty: undefined },
Expand Down Expand Up @@ -185,14 +185,14 @@ describe("EventTimeline", function() {
sentinel.name = "Old Alice";
sentinel.membership = "join";

mocked(timeline.getState(EventTimeline.FORWARDS)).getSentinelMember
mocked(timeline.getState(EventTimeline.FORWARDS)!).getSentinelMember
.mockImplementation(function(uid) {
if (uid === userA) {
return sentinel;
}
return null;
});
mocked(timeline.getState(EventTimeline.BACKWARDS)).getSentinelMember
mocked(timeline.getState(EventTimeline.BACKWARDS)!).getSentinelMember
.mockImplementation(function(uid) {
if (uid === userA) {
return oldSentinel;
Expand Down Expand Up @@ -225,14 +225,14 @@ describe("EventTimeline", function() {
sentinel.name = "Old Alice";
sentinel.membership = "join";

mocked(timeline.getState(EventTimeline.FORWARDS)).getSentinelMember
mocked(timeline.getState(EventTimeline.FORWARDS)!).getSentinelMember
.mockImplementation(function(uid) {
if (uid === userA) {
return sentinel;
}
return null;
});
mocked(timeline.getState(EventTimeline.BACKWARDS)).getSentinelMember
mocked(timeline.getState(EventTimeline.BACKWARDS)!).getSentinelMember
.mockImplementation(function(uid) {
if (uid === userA) {
return oldSentinel;
Expand Down Expand Up @@ -269,15 +269,15 @@ describe("EventTimeline", function() {
timeline.addEvent(events[0], { toStartOfTimeline: false });
timeline.addEvent(events[1], { toStartOfTimeline: false });

expect(timeline.getState(EventTimeline.FORWARDS).setStateEvents).
expect(timeline.getState(EventTimeline.FORWARDS)!.setStateEvents).
toHaveBeenCalledWith([events[0]], { timelineWasEmpty: undefined });
expect(timeline.getState(EventTimeline.FORWARDS).setStateEvents).
expect(timeline.getState(EventTimeline.FORWARDS)!.setStateEvents).
toHaveBeenCalledWith([events[1]], { timelineWasEmpty: undefined });

expect(events[0].forwardLooking).toBe(true);
expect(events[1].forwardLooking).toBe(true);

expect(timeline.getState(EventTimeline.BACKWARDS).setStateEvents).
expect(timeline.getState(EventTimeline.BACKWARDS)!.setStateEvents).
not.toHaveBeenCalled();
});

Expand All @@ -298,15 +298,15 @@ describe("EventTimeline", function() {
timeline.addEvent(events[0], { toStartOfTimeline: true });
timeline.addEvent(events[1], { toStartOfTimeline: true });

expect(timeline.getState(EventTimeline.BACKWARDS).setStateEvents).
expect(timeline.getState(EventTimeline.BACKWARDS)!.setStateEvents).
toHaveBeenCalledWith([events[0]], { timelineWasEmpty: undefined });
expect(timeline.getState(EventTimeline.BACKWARDS).setStateEvents).
expect(timeline.getState(EventTimeline.BACKWARDS)!.setStateEvents).
toHaveBeenCalledWith([events[1]], { timelineWasEmpty: undefined });

expect(events[0].forwardLooking).toBe(false);
expect(events[1].forwardLooking).toBe(false);

expect(timeline.getState(EventTimeline.FORWARDS).setStateEvents).
expect(timeline.getState(EventTimeline.FORWARDS)!.setStateEvents).
not.toHaveBeenCalled();
});

Expand Down
2 changes: 1 addition & 1 deletion spec/unit/webrtc/call.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,7 @@ describe('Call', function() {

describe("transferToCall", () => {
it("should send the required events", async () => {
const targetCall = new MatrixCall({ client: client.client });
const targetCall = new MatrixCall({ client: client.client, roomId: "!roomId:server" });
const sendEvent = jest.spyOn(client.client, "sendEvent");
await call.transferToCall(targetCall);

Expand Down
5 changes: 3 additions & 2 deletions src/@types/PushRules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export enum TweakName {

export type Tweak<N extends TweakName, V> = {
set_tweak: N;
value: V;
value?: V;
};

export type TweakHighlight = Tweak<TweakName.Highlight, boolean>;
Expand Down Expand Up @@ -76,7 +76,8 @@ export interface IPushRuleCondition<N extends ConditionKind | string> {

export interface IEventMatchCondition extends IPushRuleCondition<ConditionKind.EventMatch> {
key: string;
pattern: string;
pattern?: string;
value?: string;
}

export interface IContainsDisplayNameCondition extends IPushRuleCondition<ConditionKind.ContainsDisplayName> {
Expand Down
80 changes: 48 additions & 32 deletions src/autodiscovery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,28 @@ export enum AutoDiscoveryAction {
FAIL_ERROR = "FAIL_ERROR",
}

enum AutoDiscoveryError {
Invalid = "Invalid homeserver discovery response",
GenericFailure = "Failed to get autodiscovery configuration from server",
InvalidHsBaseUrl = "Invalid base_url for m.homeserver",
InvalidHomeserver = "Homeserver URL does not appear to be a valid Matrix homeserver",
InvalidIsBaseUrl = "Invalid base_url for m.identity_server",
InvalidIdentityServer = "Identity server URL does not appear to be a valid identity server",
InvalidIs = "Invalid identity server discovery response",
MissingWellknown = "No .well-known JSON file found",
InvalidJson = "Invalid JSON",
}

interface WellKnownConfig extends Omit<IWellKnownConfig, "error"> {
state: AutoDiscoveryAction;
error?: IWellKnownConfig["error"] | null;
}

interface ClientConfig {
"m.homeserver": WellKnownConfig;
"m.identity_server": WellKnownConfig;
}

/**
* Utilities for automatically discovery resources, such as homeservers
* for users to log in to.
Expand All @@ -42,36 +64,25 @@ export class AutoDiscovery {
// translate the meaning of the states in the spec, but also
// support our own if needed.

public static readonly ERROR_INVALID = "Invalid homeserver discovery response";
public static readonly ERROR_INVALID = AutoDiscoveryError.Invalid;

public static readonly ERROR_GENERIC_FAILURE = "Failed to get autodiscovery configuration from server";
public static readonly ERROR_GENERIC_FAILURE = AutoDiscoveryError.GenericFailure;

public static readonly ERROR_INVALID_HS_BASE_URL = "Invalid base_url for m.homeserver";
public static readonly ERROR_INVALID_HS_BASE_URL = AutoDiscoveryError.InvalidHsBaseUrl;

public static readonly ERROR_INVALID_HOMESERVER = "Homeserver URL does not appear to be a valid Matrix homeserver";
public static readonly ERROR_INVALID_HOMESERVER = AutoDiscoveryError.InvalidHomeserver;

public static readonly ERROR_INVALID_IS_BASE_URL = "Invalid base_url for m.identity_server";
public static readonly ERROR_INVALID_IS_BASE_URL = AutoDiscoveryError.InvalidIsBaseUrl;

// eslint-disable-next-line
public static readonly ERROR_INVALID_IDENTITY_SERVER = "Identity server URL does not appear to be a valid identity server";
public static readonly ERROR_INVALID_IDENTITY_SERVER = AutoDiscoveryError.InvalidIdentityServer;

public static readonly ERROR_INVALID_IS = "Invalid identity server discovery response";
public static readonly ERROR_INVALID_IS = AutoDiscoveryError.InvalidIs;

public static readonly ERROR_MISSING_WELLKNOWN = "No .well-known JSON file found";
public static readonly ERROR_MISSING_WELLKNOWN = AutoDiscoveryError.MissingWellknown;

public static readonly ERROR_INVALID_JSON = "Invalid JSON";
public static readonly ERROR_INVALID_JSON = AutoDiscoveryError.InvalidJson;

public static readonly ALL_ERRORS = [
AutoDiscovery.ERROR_INVALID,
AutoDiscovery.ERROR_GENERIC_FAILURE,
AutoDiscovery.ERROR_INVALID_HS_BASE_URL,
AutoDiscovery.ERROR_INVALID_HOMESERVER,
AutoDiscovery.ERROR_INVALID_IS_BASE_URL,
AutoDiscovery.ERROR_INVALID_IDENTITY_SERVER,
AutoDiscovery.ERROR_INVALID_IS,
AutoDiscovery.ERROR_MISSING_WELLKNOWN,
AutoDiscovery.ERROR_INVALID_JSON,
];
public static readonly ALL_ERRORS = Object.keys(AutoDiscoveryError);

/**
* The auto discovery failed. The client is expected to communicate
Expand Down Expand Up @@ -120,13 +131,13 @@ export class AutoDiscovery {
* configuration, which may include error states. Rejects on unexpected
* failure, not when verification fails.
*/
public static async fromDiscoveryConfig(wellknown: any): Promise<IClientWellKnown> {
public static async fromDiscoveryConfig(wellknown: any): Promise<ClientConfig> {
// Step 1 is to get the config, which is provided to us here.

// We default to an error state to make the first few checks easier to
// write. We'll update the properties of this object over the duration
// of this function.
const clientConfig = {
const clientConfig: ClientConfig = {
"m.homeserver": {
state: AutoDiscovery.FAIL_ERROR,
error: AutoDiscovery.ERROR_INVALID,
Expand Down Expand Up @@ -197,7 +208,7 @@ export class AutoDiscovery {
if (wellknown["m.identity_server"]) {
// We prepare a failing identity server response to save lines later
// in this branch.
const failingClientConfig = {
const failingClientConfig: ClientConfig = {
"m.homeserver": clientConfig["m.homeserver"],
"m.identity_server": {
state: AutoDiscovery.FAIL_PROMPT,
Expand Down Expand Up @@ -279,7 +290,7 @@ export class AutoDiscovery {
* configuration, which may include error states. Rejects on unexpected
* failure, not when discovery fails.
*/
public static async findClientConfig(domain: string): Promise<IClientWellKnown> {
public static async findClientConfig(domain: string): Promise<ClientConfig> {
if (!domain || typeof(domain) !== "string" || domain.length === 0) {
throw new Error("'domain' must be a string of non-zero length");
}
Expand All @@ -298,7 +309,7 @@ export class AutoDiscovery {
// We default to an error state to make the first few checks easier to
// write. We'll update the properties of this object over the duration
// of this function.
const clientConfig = {
const clientConfig: ClientConfig = {
"m.homeserver": {
state: AutoDiscovery.FAIL_ERROR,
error: AutoDiscovery.ERROR_INVALID,
Expand Down Expand Up @@ -367,18 +378,18 @@ export class AutoDiscovery {
* @return {string|boolean} The sanitized URL or a falsey value if the URL is invalid.
* @private
*/
private static sanitizeWellKnownUrl(url: string): string | boolean {
private static sanitizeWellKnownUrl(url: string): string | false {
if (!url) return false;

try {
let parsed = null;
let parsed: URL | undefined;
try {
parsed = new URL(url);
} catch (e) {
logger.error("Could not parse url", e);
}

if (!parsed || !parsed.hostname) return false;
if (!parsed?.hostname) return false;
if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return false;

const port = parsed.port ? `:${parsed.port}` : "";
Expand Down Expand Up @@ -448,12 +459,17 @@ export class AutoDiscovery {
};
}
} catch (err) {
const error = err as Error | string | undefined;
const error = err as AutoDiscoveryError | string | undefined;
let reason = "";
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
if (typeof error === "object") {
reason = (<Error>error)?.message;
}

return {
error,
raw: {},
action: AutoDiscoveryAction.FAIL_PROMPT,
reason: (<Error>error)?.message || "General failure",
reason: reason || "General failure",
};
}

Expand All @@ -463,7 +479,7 @@ export class AutoDiscovery {
action: AutoDiscoveryAction.SUCCESS,
};
} catch (err) {
const error = err as Error | string | undefined;
const error = err as Error;
return {
error,
raw: {},
Expand Down
Loading