style: enable strict and type-checked lints on eslint #228

AndyTWF · 2024-06-25T21:39:00Z

Context

Built on top of #224 which should be merged first

Description

Enables much stricter rules on eslint that also check types for increased protection.
Enables plugin:@typescript-eslint/recommended-type-checked
Enables plugin:@typescript-eslint/strict-type-checked
Enables plugin:@typescript-eslint/stylistic-type-checked

Checklist

QA'd by the author.
Unit tests created (if applicable).
Integration tests created (if applicable).
Follow coding style guidelines found here.
TypeDoc updated (if applicable).
(Optional) Update documentation for new features.

test: force typescript checks on tests

vladvelici · 2024-06-26T09:03:38Z

src/Messages.ts

@@ -238,40 +242,54 @@ export class DefaultMessages extends EventEmitter<MessageEventsMap> implements M
   * Validate the realtime message and convert it to a chat message.
   */
  private parseNewMessage(channelEventMessage: Ably.InboundMessage): Message | undefined {
-    const { data, clientId, timestamp, extras } = channelEventMessage;


Is this too evil?

Edit to explain: what's the rule that this was breaking? I know it might not be this line, but just curious what can't we do? Is it the type of extras being not specific enough?

It's because data and extras are any types, so it's trying to say that you shouldn't set any to a typed variable because that may go unnoticed and leak into your codebase:

https://typescript-eslint.io/rules/no-unsafe-assignment/

vladvelici · 2024-06-26T09:08:15Z

src/Messages.ts

@@ -238,40 +242,54 @@ export class DefaultMessages extends EventEmitter<MessageEventsMap> implements M
   * Validate the realtime message and convert it to a chat message.
   */
  private parseNewMessage(channelEventMessage: Ably.InboundMessage): Message | undefined {
-    const { data, clientId, timestamp, extras } = channelEventMessage;
+    interface MessageCreatedAblyMessage {


Consistency with Reactions again? MessagePayload?

vladvelici · 2024-06-26T09:10:42Z

src/SubscriptionManager.ts

@@ -56,7 +56,8 @@ export class DefaultSubscriptionManager implements SubscriptionManager {
          channel: this._channel.name,
        });
        this._presenceEntered = false;
-        this.detachChannelIfNotListening().then(() => {});
+        // eslint-disable-next-line @typescript-eslint/no-empty-function
+        void this.detachChannelIfNotListening().then(() => {});


Do we have to listen to the result of this promise at all if we just pass an empty function?

If we still need to pass an empty function can we define it outside of the class so it won't be a closure for the DefaultSubscriptionManager instance?

There's a rule that says you shouldn't just have a dangling promise that you do nothing with the result of, so you should have a then or a catch etc, or explicitly mark it as void.

I've just done the latter so the then can be ignored :)

In any case, SubscriptionManager will be saying bye bye in the next few days, so we don't have to worry too much :D

https://typescript-eslint.io/rules/no-floating-promises/ is the rule

vladvelici · 2024-06-26T09:21:25Z

src/logger.ts

@@ -168,7 +166,8 @@ class DefaultLogger implements Logger {
      throw new Error(`Invalid log level: ${level}`);
    }

-    this._levelNumber = logLevelNumberMap.get(level) as LogLevelNumbers;
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+    this._levelNumber = logLevelNumberMap.get(level)!;


What about skipping the ! altogether

const levelNumber = logLevelNumberMap.get(level); if (levelNumber === undefined) { throw new Error(`Invalid log level: ${level}`); } this._levelNumber = levelNumber;

Not sure why I didn't think of that first time! Done!

vladvelici · 2024-06-26T09:22:55Z

src/utils/EventEmitter.ts

@@ -28,7 +28,7 @@ export function removeListener<T>(
  let eventName: keyof T;

  for (let targetListenersIndex = 0; targetListenersIndex < targetListeners.length; targetListenersIndex++) {
-    listeners = targetListeners[targetListenersIndex];
+    listeners = targetListeners[targetListenersIndex]!;


that's odd, I thought it would figure out that that's an OK thing to do.

Did we turn up the evil too much?

It's technically not wrong 😅

We define listeners a few lines above and don't include undefined, targetListeners[idx] can be undefined, so it rightly points out the error. I've added | undefined to the def of listeners and that solves all our woes!

It was purely a product of our own doing 😆

Enables much stricter rules on eslint that also check types for increased protection.

vladvelici · 2024-06-26T14:22:06Z

src/ChatApi.ts

    }

-    const [result] = response.items;
+    const [result] = response.items as RES[];


Call this.realtime.request<RES>(...) instead of casting here?

The const [result] stuff was here before, but I was wondering why don't we just reponse.items[0]!?

Attempted to tidy this a bit :)

vladvelici · 2024-06-26T14:25:38Z

src/Presence.ts

@@ -358,7 +371,7 @@ export class DefaultPresence extends EventEmitter<PresenceEventsMap> implements
        50000,
        500,
        (error as Error).message,
-      );
+      ) as unknown as Error;


I'm not sure what's going on here, but it's odd to have to as unknown as Error all over the place. Can we make this not required or add ticket to make it so later?

Maybe a potential fix is to make a constructor function that does the casting?

This is because ErrorInfo doesn't extend Error in the ably-js types :(

I've already merged the fix in ably/ably-js#1805 so we can remove this in the next version of ably-js

vladvelici · 2024-06-26T14:30:46Z

src/Rooms.ts

@@ -90,6 +91,10 @@ export class DefaultRooms implements Rooms {
    if (!room) {
      return;
    }
+
+    // eslint-disable-next-line @typescript-eslint/no-dynamic-delete


Should we use a Map instead so we don't have to disable-next-line?

A very good shout, I've done this!

vladvelici · 2024-06-26T14:49:27Z

src/Typing.ts

@@ -160,6 +160,7 @@ export class DefaultTyping extends EventEmitter<TypingEventsMap> implements Typi
   */
  private startTypingTimer(): void {
    this._logger.trace(`DefaultTyping.startTypingTimer();`);
+    // eslint-disable-next-line @typescript-eslint/no-misused-promises
    this._timerId = setTimeout(async () => {


As far as I know, this function shouldn't be async because nothing will await it.

The stop() also shouldn't be async because it doesn't await anything, it just returns a promise.

I think to please eslint we have to void this.stop(); but generally we need to think about the logic here. If the this._managedChannel.presenceLeaveClient(this._clientId); inside stop() fails the error is silenced without warning.

Yeah agreed - propose we defer the latter to the upcoming listeners changes?

vladvelici

This is certainly an improvement in the right direction. I've left some comments inline, but I think we generally want to not need any eslint-disable-next-line comments (or very few where they make sense).

This can be something we improve over time, and just leave them in some places for now. Merging this PR sooner rather than later ensures new code is up to "eslint evil" standard.

Most fixes are small but they add up in time to do in bulk, test, etc. Given our timeline maybe we should add a ticket for later to go through all these places?

I haven't reviewed 100% of this.

AndyTWF · 2024-06-26T15:02:18Z

Most fixes are small but they add up in time to do in bulk, test, etc. Given our timeline maybe we should add a ticket for later to go through all these places?

Agree - I think we need to enable strict eslint moving forwards to ensure quality of future code, but we can come back to the ignore comments at a future time and decide how best to handle them :)

vladvelici · 2024-06-26T15:03:15Z

src/ChatApi.ts

    }

-    const [result] = response.items;
-    return result as RES;
+    return [response.items[0]] as RES;


Previously we were returning a single element and now an array? We could just return response.items if we want the array, or reponse.items[0]! if we want the first element (and we're sure there is a first element)?

I'm not sure I'm reading this right, the modern js-fu is 🤯 sometimes.

No you're right, I messed up that one - fixed now!

github-actions bot temporarily deployed to staging/pull/228/typedoc June 25, 2024 21:39 Inactive

AndyTWF requested review from vladvelici and splindsay-92 and removed request for vladvelici June 25, 2024 21:43

AndyTWF force-pushed the test-typing branch from 4321216 to e6ea6bd Compare June 26, 2024 08:48

Merge pull request #224 from ably-labs/test-typing

c4b4b1a

test: force typescript checks on tests

AndyTWF force-pushed the test-typing-evil-eslint branch from 4d240eb to 90be0df Compare June 26, 2024 08:54

github-actions bot temporarily deployed to staging/pull/228/typedoc June 26, 2024 08:55 Inactive

vladvelici reviewed Jun 26, 2024

View reviewed changes

AndyTWF force-pushed the test-typing-evil-eslint branch from 90be0df to ac6ec14 Compare June 26, 2024 09:13

github-actions bot temporarily deployed to staging/pull/228/typedoc June 26, 2024 09:14 Inactive

vladvelici reviewed Jun 26, 2024

View reviewed changes

AndyTWF force-pushed the test-typing-evil-eslint branch from ac6ec14 to 0dd682a Compare June 26, 2024 10:16

github-actions bot temporarily deployed to staging/pull/228/typedoc June 26, 2024 10:17 Inactive

AndyTWF force-pushed the test-typing-evil-eslint branch from 0dd682a to 6430e01 Compare June 26, 2024 10:20

github-actions bot temporarily deployed to staging/pull/228/typedoc June 26, 2024 10:20 Inactive

style: enable strict and type-checked lints on eslint

f4f9017

Enables much stricter rules on eslint that also check types for increased protection.

AndyTWF force-pushed the test-typing-evil-eslint branch from 6430e01 to f4f9017 Compare June 26, 2024 10:23

AndyTWF requested a review from vladvelici June 26, 2024 10:23

github-actions bot temporarily deployed to staging/pull/228/typedoc June 26, 2024 10:23 Inactive

vladvelici reviewed Jun 26, 2024

View reviewed changes

rooms: make rooms a map

5b5c199

AndyTWF requested a review from vladvelici June 26, 2024 14:44

github-actions bot temporarily deployed to staging/pull/228/typedoc June 26, 2024 14:44 Inactive

vladvelici reviewed Jun 26, 2024

View reviewed changes

github-actions bot temporarily deployed to staging/pull/228/typedoc June 26, 2024 15:02 Inactive

vladvelici reviewed Jun 26, 2024

View reviewed changes

AndyTWF added 2 commits June 26, 2024 16:07

chatapi: type tidying

896f3d4

typing: use void promise in timeout

59cd92b

AndyTWF force-pushed the test-typing-evil-eslint branch from 2840f8e to 59cd92b Compare June 26, 2024 15:07

github-actions bot deployed to staging/pull/228/typedoc June 26, 2024 15:08 View deployment

vladvelici approved these changes Jun 26, 2024

View reviewed changes

AndyTWF merged commit c35e768 into test-typing Jun 26, 2024
8 checks passed

AndyTWF mentioned this pull request Jun 26, 2024

style: eslint stricter typing #231

Merged

github-actions bot temporarily deployed to staging/pull/231/typedoc June 26, 2024 15:16 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: enable strict and type-checked lints on eslint #228

style: enable strict and type-checked lints on eslint #228

AndyTWF commented Jun 25, 2024 •

edited

Loading

vladvelici Jun 26, 2024 •

edited

Loading

AndyTWF Jun 26, 2024

vladvelici Jun 26, 2024

AndyTWF Jun 26, 2024

vladvelici Jun 26, 2024

AndyTWF Jun 26, 2024 •

edited

Loading

AndyTWF Jun 26, 2024

vladvelici Jun 26, 2024

AndyTWF Jun 26, 2024

vladvelici Jun 26, 2024

AndyTWF Jun 26, 2024 •

edited

Loading

vladvelici Jun 26, 2024

AndyTWF Jun 26, 2024

vladvelici Jun 26, 2024

AndyTWF Jun 26, 2024

vladvelici Jun 26, 2024

vladvelici Jun 26, 2024

AndyTWF Jun 26, 2024

vladvelici Jun 26, 2024

AndyTWF Jun 26, 2024

vladvelici Jun 26, 2024

vladvelici left a comment •

edited

Loading

AndyTWF commented Jun 26, 2024

vladvelici Jun 26, 2024

vladvelici Jun 26, 2024

AndyTWF Jun 26, 2024

style: enable strict and type-checked lints on eslint #228

style: enable strict and type-checked lints on eslint #228

Conversation

AndyTWF commented Jun 25, 2024 • edited Loading

Context

Description

Checklist

vladvelici Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyTWF Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyTWF Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladvelici left a comment • edited Loading

Choose a reason for hiding this comment

AndyTWF commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyTWF commented Jun 25, 2024 •

edited

Loading

vladvelici Jun 26, 2024 •

edited

Loading

AndyTWF Jun 26, 2024 •

edited

Loading

AndyTWF Jun 26, 2024 •

edited

Loading

vladvelici left a comment •

edited

Loading