Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Remove references to internal js-sdk type CryptoBackend #12321

Merged
merged 2 commits into from
Mar 6, 2024
Merged

Remove references to internal js-sdk type CryptoBackend #12321

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79c98a3
Remove references to internal js-sdk type `CryptoBackend`
richvdh Mar 6, 2024
094be3c
Use `Paramteters` to avoid `ts-ignore`
richvdh Mar 6, 2024
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
2 changes: 0 additions & 2 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,6 @@ module.exports = {
"!matrix-js-sdk/src/crypto/verification/QRCode",
"!matrix-js-sdk/src/crypto/verification/request",
"!matrix-js-sdk/src/crypto/verification/request/VerificationRequest",
"!matrix-js-sdk/src/common-crypto",
"!matrix-js-sdk/src/common-crypto/CryptoBackend",
"!matrix-js-sdk/src/oidc",
"!matrix-js-sdk/src/oidc/discovery",
"!matrix-js-sdk/src/oidc/authorize",
Expand Down
6 changes: 3 additions & 3 deletions test/components/views/rooms/EventTile-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
TweakName,
} from "matrix-js-sdk/src/matrix";
import { EventEncryptionInfo, EventShieldColour, EventShieldReason } from "matrix-js-sdk/src/crypto-api";
import { CryptoBackend } from "matrix-js-sdk/src/common-crypto/CryptoBackend";
import { TooltipProvider } from "@vector-im/compound-web";

import EventTile, { EventTileProps } from "../../../../src/components/views/rooms/EventTile";
Expand Down Expand Up @@ -317,11 +316,12 @@ describe("EventTile", () => {
});

const mockCrypto = {
decryptEvent: async (_ev): Promise<IEventDecryptionResult> => {
decryptEvent: async (_ev: MatrixEvent): Promise<IEventDecryptionResult> => {
throw new Error("can't decrypt");
},
} as CryptoBackend;
};

// @ts-ignore `mockCrypto` is not a `CryptoBackend`, which is an internal type in the js-sdk
Copy link
Member

@t3chguy t3chguy Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This makes development harder as we won't get errors when the types for the provided fields are wrong (it'll tell you to cast to unknown first if sure) and are left to fend for our own, if we're not going to use the types then why have them in the first place. It can and will result in tests which end up testing things not conforming to interfaces, providing incorrect return types and the like. We should not be using ts-ignore anywhere if we can help it, I'm surprised it isn't linted against at this layer like we do at other layers

image

Copy link
Member

Choose a reason for hiding this comment

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

Could use Parameters<typeof MatrixEvent["attemptDecryption"]>[0] as an alternative for CryptoBackend

Copy link
Member Author

Choose a reason for hiding this comment

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

if we're not going to use the types then why have them in the first place

This is an odd question. We have the type so that we can use it in the internals of js-sdk. The fact that a library defines a type in its internals does not necessarily mean that every application using that library can and should be using that type. Indeed, exposing those types makes a library way harder to use because it means application authors have to read through way more documentation to find the thing they are actually supposed to use.

The fundamental problem here is that we are poking into the innards of js-sdk in order to make a UTD event. Really, the problem is that we are calling MatrixEvent.attemptDecryption, which is an internal method (and yes, it's parameter is an iternal type). But I don't really have any great ideas for avoiding that.

It can and will result in tests which end up testing things not conforming to interfaces, providing incorrect return types and the like.

Mayyyybe? The existing code doesn't actually implement CryptoBackend; it just mocks out one function for it. Who is to say that attemptDecryption doesn't care about any of the rest of the interface? And once you're mocking a function, its declared interface seems the least of your problems: what it actually does is far more important for whether the test is realistic.

I would strongly argue that having a @ts-ignore in test code is a lesser evil than making CryptoBackend part of js-sdk's public interface.

Anyway, I have applied your suggestion, which seems to solve the problem here.

await mxEvent.attemptDecryption(mockCrypto);

const { container } = getComponent();
Expand Down
6 changes: 3 additions & 3 deletions test/test-utils/test-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import { normalize } from "matrix-js-sdk/src/utils";
import { ReEmitter } from "matrix-js-sdk/src/ReEmitter";
import { MediaHandler } from "matrix-js-sdk/src/webrtc/mediaHandler";
import { Feature, ServerSupport } from "matrix-js-sdk/src/feature";
import { CryptoBackend } from "matrix-js-sdk/src/common-crypto/CryptoBackend";
import { MapperOpts } from "matrix-js-sdk/src/event-mapper";
// eslint-disable-next-line no-restricted-imports
import { MatrixRTCSessionManager } from "matrix-js-sdk/src/matrixrtc/MatrixRTCSessionManager";
Expand Down Expand Up @@ -441,9 +440,10 @@ export async function mkEncryptedEvent(opts: {
};

const mockCrypto = {
decryptEvent: async (_ev): Promise<IEventDecryptionResult> => decryptionResult,
} as CryptoBackend;
decryptEvent: async (_ev: MatrixEvent): Promise<IEventDecryptionResult> => decryptionResult,
};

// @ts-ignore `mockCrypto` is not a `CryptoBackend`, which is an internal type in the js-sdk
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

await mxEvent.attemptDecryption(mockCrypto);
return mxEvent;
}
Expand Down
Loading