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

Have js-sdk pass TSconfig strict: true #2116

Closed
5 tasks
ShadowJonathan opened this issue Jan 23, 2022 · 2 comments · Fixed by #2835
Closed
5 tasks

Have js-sdk pass TSconfig strict: true #2116

ShadowJonathan opened this issue Jan 23, 2022 · 2 comments · Fixed by #2835

Comments

@ShadowJonathan
Copy link
Contributor

This will remove tons of bugs simply by virtue of function types becoming more correct.

Sub-issues;

Related: #2115

@ShadowJonathan ShadowJonathan changed the title Have js-sdk pass TS eslint strict: true Have js-sdk pass TSconfig strict: true Jan 23, 2022
@turt2live
Copy link
Member

What are the benefits to doing this? I've yet to be convinced that strict: true is a good idea :/

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Jan 24, 2022

strict: true will enable 8 other linters by default, this issue is meant to be partially a tracker for these as well.

All of these linters make TypeScript apply more scrutiny and static analysis to the TS source files, which in turn would make the overall program more deterministic and type-safe.

These are the applied linters, i'll explain each in turn;

  • alwaysStrict
  • strictNullChecks
  • strictBindCallApply
  • strictFunctionTypes
  • strictPropertyInitialization
  • noImplicitAny
  • noImplicitThis
  • useUnknownInCatchVariables

alwaysStrict

This'll make Typescript emit its javascript in strict mode, and type-check it the same way as it would be in strict mode.

This has a number of advantages, all of which are listed here, but a few highlights;

  • Unique function parameters
  • Throw on assignment to get/readonly property
  • Throw on assignment to a nonasignable global (undefined, Infinity, etc.)

Ultimately, the lint description highlights it as thus;

ECMAScript strict mode was introduced in ES5 and provides behavior tweaks to the runtime of the JavaScript engine to improve performance, and makes a set of errors throw instead of silently ignoring them.

strictNullChecks

This is explained in #2112 (comment).

strictBindCallApply

Will apply stricter checking to .call, .apply, and .bind, making sure that the types given to its arguments are the same as given on the function signature.

If this is not enabled, these calls will not type-check, and also not inherit return types from the function, instead returning any.

strictFunctionTypes

This will more strictly interpret function types, making sure that functions are unassignable if the defined function type cannot be completely "covered" by the function-to-be-assigned, example;

function fn(x: string) {
  console.log("Hello, " + x.toLowerCase());
}
 
type StringOrNumberFunc = (ns: string | number) => void;
 
// Unsafe assignment
let func: StringOrNumberFunc = fn;
// Unsafe call - will crash
func(10);

With this checker on, the above will raise a type error during linting, without it, it will be silently ignored.

#2114 is a derivative of this, making sure that every interface will have its functions applied more correctly.

strictPropertyInitialization

This will make sure that any and all properties on a class are initialized on construction.

Example, below, every property is "accounted for" during construction, except email, which will erroneously become undefined after construction.

class UserAccount {
  name: string;
  accountType = "user";
 
  email: string;
  address: string | undefined;
 
  constructor(name: string) {
    this.name = name;
    // Note that this.email is not set
  }
}

noImplicitAny

This will, in essence, make sure that every function parameter is property type-annotated.

This'd disallow the following, and raise an error when encountering it;

function doThis(thing) {
  // what type is thing???
  thing.doSomething() // not type-checked, as `thing` is `any`
}

noImplicitThis

This one is more subtle, but best explained with the following example;

class Rectangle {
  width: number;
  height: number;
 
  constructor(width: number, height: number) {
    this.width = width;
    this.height = height;
  }
 
  getAreaFunction() {
    return function () {
      return this.width * this.height;
      // error: 'this' implicitly has type 'any' because it does not have a type annotation. 
    };
  }
}

This makes sure that every this invocation is - at the very least - definitively typed.

useUnknownInCatchVariables

This will, in essense, treat the e from every catch(e) as unknown.

unknown and any share similar traits, but a basic one is that, while any is assignable to any type, unknown is assignable to none, bar itself. This means that let err: Error = e; will not work, an explicit cast like as Error is needed to override the invariant.

There is a good reason that this must be the case in catch, as any function down the call stack can throw any sort of value up the chain, so when this arrives, typescript cannot check anything, it is up to the programmer to start applying type guards ("let me check this first") or type assertions ("i know what i'm doing, treat this variable as this other type").

try {
  // ...
} catch (err) {
  // We have to verify err is an
  // error before using it as one.
  if (err instanceof Error) {
    console.log(err.message);
  }
}

err here would remain unknown until TS notices us adding a type guard, after which it'll treat err as Error.


This issue was made with the idea that all of these checks should've been implemented into js-sdk "yesterday", that these checks are needed to keep a large codebase in check, and make sure all of its behavior is (more) deterministic.

The alternative would be that this behaviour would be unchecked, such behaviour would then break the basics of TS' type safety, creating a false sense of security, and leading to headaches, programmer bugs, runtime nullability problems (propagated undefined and thrown errors on calling undefined as a function), and at worst, vulnerabilities.

su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this issue Nov 25, 2022
* Make calls go back to 'connecting' state when media lost ([\matrix-org#2880](matrix-org#2880)).
* Add ability to send unthreaded receipt ([\matrix-org#2878](matrix-org#2878)).
* Add way to abort search requests ([\matrix-org#2877](matrix-org#2877)).
* sliding sync: add custom room subscriptions support ([\matrix-org#2834](matrix-org#2834)).
* webrtc: add advanced audio settings ([\matrix-org#2434](matrix-org#2434)). Contributed by @MrAnno.
* Add support for group calls using MSC3401 ([\matrix-org#2553](matrix-org#2553)).
* Make the js-sdk conform to tsc --strict ([\matrix-org#2835](matrix-org#2835)). Fixes matrix-org#2112 matrix-org#2116 and matrix-org#2124.
* Let leave requests outlive the window ([\matrix-org#2815](matrix-org#2815)). Fixes element-hq/element-call#639.
* Add event and message capabilities to RoomWidgetClient ([\matrix-org#2797](matrix-org#2797)).
* Misc fixes for group call widgets ([\matrix-org#2657](matrix-org#2657)).
* Support nested Matrix clients via the widget API ([\matrix-org#2473](matrix-org#2473)).
* Set max average bitrate on PTT calls ([\matrix-org#2499](matrix-org#2499)). Fixes element-hq/element-call#440.
* Add config option for e2e group call signalling ([\matrix-org#2492](matrix-org#2492)).
* Enable DTX on audio tracks in calls ([\matrix-org#2482](matrix-org#2482)).
* Don't ignore call member events with a distant future expiration date ([\matrix-org#2466](matrix-org#2466)).
* Expire call member state events after 1 hour ([\matrix-org#2446](matrix-org#2446)).
* Emit unknown device errors for group call participants without e2e ([\matrix-org#2447](matrix-org#2447)).
* Mute disconnected peers in PTT mode ([\matrix-org#2421](matrix-org#2421)).
* Add support for sending encrypted to-device events with OLM ([\matrix-org#2322](matrix-org#2322)). Contributed by @robertlong.
* Support for PTT group call mode ([\matrix-org#2338](matrix-org#2338)).
* Fix registration add phone number not working ([\matrix-org#2876](matrix-org#2876)). Contributed by @bagvand.
* Use an underride rule for Element Call notifications ([\matrix-org#2873](matrix-org#2873)). Fixes element-hq/element-web#23691.
* Fixes unwanted highlight notifications with encrypted threads ([\matrix-org#2862](matrix-org#2862)).
* Extra insurance that we don't mix events in the wrong timelines - v2 ([\matrix-org#2856](matrix-org#2856)). Contributed by @MadLittleMods.
* Hide pending events in thread timelines ([\matrix-org#2843](matrix-org#2843)). Fixes element-hq/element-web#23684.
* Fix pagination token tracking for mixed room timelines ([\matrix-org#2855](matrix-org#2855)). Fixes element-hq/element-web#23695.
* Extra insurance that we don't mix events in the wrong timelines ([\matrix-org#2848](matrix-org#2848)). Contributed by @MadLittleMods.
* Do not freeze state in `initialiseState()` ([\matrix-org#2846](matrix-org#2846)).
* Don't remove our own member for a split second when entering a call ([\matrix-org#2844](matrix-org#2844)).
* Resolve races between `initLocalCallFeed` and `leave` ([\matrix-org#2826](matrix-org#2826)).
* Add throwOnFail to groupCall.setScreensharingEnabled ([\matrix-org#2787](matrix-org#2787)).
* Fix connectivity regressions ([\matrix-org#2780](matrix-org#2780)).
* Fix screenshare failing after several attempts ([\matrix-org#2771](matrix-org#2771)). Fixes element-hq/element-call#625.
* Don't block muting/unmuting on network requests ([\matrix-org#2754](matrix-org#2754)). Fixes element-hq/element-call#592.
* Fix ICE restarts ([\matrix-org#2702](matrix-org#2702)).
* Target widget actions at a specific room ([\matrix-org#2670](matrix-org#2670)).
* Add tests for ice candidate sending ([\matrix-org#2674](matrix-org#2674)).
* Prevent exception when muting ([\matrix-org#2667](matrix-org#2667)). Fixes element-hq/element-call#578.
* Fix race in creating calls ([\matrix-org#2662](matrix-org#2662)).
* Add client.waitUntilRoomReadyForGroupCalls() ([\matrix-org#2641](matrix-org#2641)).
* Wait for client to start syncing before making group calls ([\matrix-org#2632](matrix-org#2632)). Fixes matrix-org#2589.
* Add GroupCallEventHandlerEvent.Room ([\matrix-org#2631](matrix-org#2631)).
* Add missing events from reemitter to GroupCall ([\matrix-org#2527](matrix-org#2527)). Contributed by @toger5.
* Prevent double mute status changed events ([\matrix-org#2502](matrix-org#2502)).
* Don't mute the remote side immediately in PTT calls ([\matrix-org#2487](matrix-org#2487)). Fixes element-hq/element-call#425.
* Fix some MatrixCall leaks and use a shared AudioContext ([\matrix-org#2484](matrix-org#2484)). Fixes element-hq/element-call#412.
* Don't block muting on determining whether the device exists ([\matrix-org#2461](matrix-org#2461)).
* Only clone streams on Safari ([\matrix-org#2450](matrix-org#2450)). Fixes element-hq/element-call#267.
* Set PTT mode on call correctly ([\matrix-org#2445](matrix-org#2445)). Fixes element-hq/element-call#382.
* Wait for mute event to send in PTT mode ([\matrix-org#2401](matrix-org#2401)).
* Handle other members having no e2e keys ([\matrix-org#2383](matrix-org#2383)). Fixes element-hq/element-call#338.
* Fix races when muting/unmuting ([\matrix-org#2370](matrix-org#2370)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants