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

Fix type errors in README.md #3766

Closed
wants to merge 2 commits into from

Conversation

jameshfisher
Copy link

@jameshfisher jameshfisher commented Oct 2, 2023

Most examples in the README have type errors have type errors when using "matrix-js-sdk": "^28.2.0". There are several causes:

  • Passing plain strings (e.g. "Room.timeline") as event names does not type-check, because the event names are defined using a TypeScript enum. Instead, one must use sdk.RoomEvent.Timeline etc.
  • The examples imply callback-based APIs that seem to no longer exist. For example, client.publicRooms does not take a callback; instead it returns a Promise.
  • The examples assume some values that, according to the types, can be null or undefined.
  • The examples use some accessors like client.store.rooms that do not exist in the types; instead one must use getters like client.store.getRooms().

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

Most examples in the README have type errors have type errors when using `"matrix-js-sdk": "^28.2.0"`. There are several causes:

* Passing plain strings (e.g. `"Room.timeline"`) as event names does not type-check, because the event names are defined using a TypeScript `enum`. Instead, one must use `sdk.RoomEvent.Timeline` etc.
* The examples imply callback-based APIs that seem to no longer exist. For example, `client.publicRooms` does not take a callback; instead it returns a `Promise`.
* The examples assume some values that, according to the types, can be `null` or `undefined`.
* The examples use some accessors like `client.store.rooms` that do not exist in the types; instead one must use getters like `client.store.getRooms()`.
@jameshfisher jameshfisher requested a review from a team as a code owner October 2, 2023 13:37
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Oct 2, 2023
@t3chguy
Copy link
Member

t3chguy commented Oct 2, 2023

The examples are JS rather than TS where the strings continue to work just fine

@jameshfisher
Copy link
Author

The examples are JS rather than TS where the strings continue to work just fine

My assumption is that you want JS that is not just runtime-correct, but also type-correct according to your own typings. If they're not, most people trying to get started will assume (as I did) that the code is broken. (I believe even the developers writing JS will be using the TypeScript typings, because IDEs use these published typings even in JS.)

The approach I took is to fix the examples to work with the typings. The alternative is to fix the types to work with the examples. The problem is that matrix-js-sdk is using TypeScript's enum, which should generally be avoided (for this reason, among others!).

For example, this is how ClientEvent is currently defined:

export enum ClientEvent {
    Sync = "sync",
    Event = "event",
    // ...
    TurnServers = "turnServers",
    TurnServersError = "turnServers.error",
}

Using this definition, this does not type-check:

// Error: Type '"sync"' is not assignable to type 'ClientEvent'
const ev: sdk.ClientEvent = "sync";

If you want this to work, a better definition would be:

export const ClientEvent = {
    Sync: "sync",
    Event: "event",
    // ...
    TurnServers: "turnServers",
    TurnServersError: "turnServers.error",
} as const;

export type ClientEvent = typeof ClientEvent[keyof typeof ClientEvent]; // "sync" | "event" | ... 

So - do the team instead want to fix the types to work with the examples?

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane, thanks

@t3chguy t3chguy added the T-Task Tasks for the team like planning label Oct 16, 2023
@richvdh richvdh requested a review from t3chguy December 5, 2023 13:48
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

@jameshfisher the linter isn't happy with you, please fix it and request a re-review

@richvdh
Copy link
Member

richvdh commented Nov 6, 2024

closing this as it seems to have bitrotted.

@richvdh richvdh closed this Nov 6, 2024
auto-merge was automatically disabled November 6, 2024 11:42

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants