[MMB-156] Prepare for adding documentation comments #197

lawrence-forooghian · 2023-09-28T19:38:02Z

This introduces an assortment of refactorings intended to make it easier to perform MMB-156 (adding documentation comments). See commit messages for more details.

dpiatek · 2023-10-06T09:52:26Z

src/Locations.ts

@@ -11,8 +11,16 @@ import type Space from './Space.js';
 import { ERR_NOT_ENTERED_SPACE } from './Errors.js';
 import SpaceUpdate from './SpaceUpdate.js';

+export namespace LocationsEvents {


I'd prefer that we do not use namespaces if possible. Namespaces, and enums, are two features of TS that break it's main advantage - of adding just types, not features (there is no namespaces or enums in JS). Could we use a module with named exports for this?

Would you mind explaining your objection to my use of namespaces in this case, given that I'm using them as a way to organise types?

I am also happy to change it, but I don't think I understand what you mean by using a module, would you mind showing me an example?

hey @lawrence-forooghian thought about this and I think it's ok to use them just for types.

My objection for using namespaces is that they do not exist in JavaScript and so if you add code into them they will have to compile into something else. They were deprecated at some point in TS but I think that decision was reversed (because of how many people use them maybe). Apart from them not existing in JavaScript, the main issue is they duplicate what modules do (ie. organise code, namespace methods and properties).

dpiatek · 2023-10-06T10:02:05Z

src/types.ts

@@ -39,7 +39,13 @@ export interface SpaceMember {
  };
 }

-export type LockStatus = 'pending' | 'locked' | 'unlocked';
+export namespace LockStatuses {


As per the other comment about namespaces - can avoid using them please.

As there, would you mind showing me a suggestion of how to replace them? Thanks!

(For context, here I tried to stay consistent with the approach of ably-js, which makes heavy use of namespaces in its typings).

dpiatek · 2023-10-06T10:58:09Z

src/utilities/EventEmitter.ts

@@ -164,7 +164,7 @@ export default class EventEmitter<T> {
    }

    // .off("eventName", () => {})
-    if (isString(listenerOrEvents) && isFunction(listener)) {
+    if (isString(listenerOrEvents) && listener) {


@lawrence-forooghian let's please bring this back. Numerous of our customers are actually not using typescript. I think it's better to be explicit here and not rely purely on the TS.

I've removed this commit.

dpiatek · 2023-10-06T11:02:04Z

src/utilities/EventEmitter.ts

-export type EventMap = Record<string, unknown>;
-// extract all the keys of an event map and use them as a type
-export type EventKey<T extends EventMap> = string & keyof T;
-export type EventListener<T> = (params: T) => void;


I'd like to discuss these changes offline

Approved by mistake

dpiatek

This is some great work @lawrence-forooghian, thank you very much. There is some points I'd like to discuss but I don't think this is far off.

Make them all of the form "EventMap" prefixed by the name of the class that emits them. Note that the use of "EventMap" as opposed to "EventsMap" naming is also consistent with the naming used by TypeScript for the DOM event maps (e.g. [1]). [1] https://github.com/microsoft/TypeScript/blob/f3dad2a07d007c40d260f13d6b087a2d5685d12d/src/lib/dom.generated.d.ts#L2418-L2422

I am shortly going to be adding documentation comments to the codebase, but only for public API. I’ll then use TypeDoc to generate documentation based on these comments. So, to avoid TypeDoc errors of the form "X does not have any documentation" for non-public API, I’m marking the non-public API as `private` or `@internal`, and will configure TypeDoc to ignore any such-marked items. My guessing of what constitutes public API is done based on what’s mentioned in the documentation on ably.com and what’s used in the demo code bundled in this repo.

As mentioned in 9f9ff98, I’ll be adding TypeDoc to the project. The aim of the change in this commit is to avoid TypeDoc warnings of the form "X (...) is referenced by Y but not included in the documentation."

So that I can write documentation comments for them.

I want to convert the *EventMap types from types to interfaces, but the EventMap type complicates this. So, let’s get rid of it and the accompanying EventKey type. I can’t see a compelling reason for their existence. Removing them also means fewer types for users to understand, and fewer types for us to document.

Change `listener`’s type from Function to EventListener, and make it clearer that an EventListener takes a single argument.

So that I can use {@link} in documentation comments to link to the documentation of each of their properties.

So that they can be individually documented.

The subscribe, unsubscribe, … etc methods are easier to understand (and easier to document) when thought of as two variants: one which accepts one or more event names, and one which doesn’t. So create these overload signatures.

Make it clear, via the type system, that a listener can access the name of the emitted event via the `this` variable.

Easier to document.

Motivation as 97f5ac2.

Elsewhere in the codebase we’ve only used `K` for types related to keys.

To make it easier to document.

I didn’t really want to do this, but with the TypeDoc configuration that I intend to use (i.e. with `"validation": true` and `requiredToBeDocumented` populated with all types) it seems unavoidable, else I get an error of the form > [warning] Space.updateProfileData.updateFn.__type, defined in ./dist/mjs/Space.d.ts, does not have any documentation. I could not figure out how to satisfy this warning without creating a type for this callback. I think that in a codebase which makes heavy use of callbacks this would not be ideal, but I guess we can live with it in our codebase, where there are few callbacks. The alternative would be to try and change the TypeDoc configuration, but I couldn’t find a non-trivial requiredToBeDocumented value which made the warning go away. And I don’t really want to make requiredToBeDocumented trivial nor turn off the `validation` option. It’s a shame that, as far as I can tell, TypeDoc doesn’t offer the option of suppressing a warning for a single symbol.

I haven’t seen this capitalisation used elsewhere, and I don’t think the fragment is appropriate.

lawrence-forooghian marked this pull request as ready for review September 28, 2023 19:45

lawrence-forooghian requested a review from dpiatek September 28, 2023 19:45

lawrence-forooghian mentioned this pull request Sep 28, 2023

(WIP) [MMB-156] Add documentation comments and generate HTML documentation #191

Closed

lawrence-forooghian force-pushed the prepare-for-documentation-comments branch from 686136b to beb605c Compare September 28, 2023 19:55

lawrence-forooghian force-pushed the remove-EventEmitter-once-multiple-events branch from e4a4641 to 9137e98 Compare October 2, 2023 12:47

lawrence-forooghian force-pushed the prepare-for-documentation-comments branch 11 times, most recently from f3523c0 to 562abfe Compare October 3, 2023 13:01

lawrence-forooghian mentioned this pull request Oct 4, 2023

[MMB-156] Add documentation comments and generate HTML documentation #204

Merged

dpiatek force-pushed the remove-EventEmitter-once-multiple-events branch 2 times, most recently from ab13900 to eee7507 Compare October 6, 2023 09:09

dpiatek previously approved these changes Oct 6, 2023

View reviewed changes

dpiatek suggested changes Oct 6, 2023

View reviewed changes

Base automatically changed from remove-EventEmitter-once-multiple-events to main October 9, 2023 10:50

lawrence-forooghian force-pushed the prepare-for-documentation-comments branch from 1234271 to 0dc5de5 Compare October 9, 2023 13:13

dpiatek approved these changes Oct 16, 2023

View reviewed changes

dpiatek force-pushed the prepare-for-documentation-comments branch from 0dc5de5 to 20ef7aa Compare October 16, 2023 09:11

lawrence-forooghian added 4 commits October 16, 2023 08:52

Remove unused type Provider

473ca77

Export all types that are referenced by exported types

9dc816c

As mentioned in 9f9ff98, I’ll be adding TypeDoc to the project. The aim of the change in this commit is to avoid TypeDoc warnings of the form "X (...) is referenced by Y but not included in the documentation."

lawrence-forooghian added 14 commits October 16, 2023 08:53

Extract a couple of events to their own types

3b78093

So that I can write documentation comments for them.

Improve typing of callListener function

705ea95

Change `listener`’s type from Function to EventListener, and make it clearer that an EventListener takes a single argument.

Convert *EventMap types to interfaces

28067d5

So that I can use {@link} in documentation comments to link to the documentation of each of their properties.

Split LockStatus’s allowed values into separate types

bd7f5d7

So that they can be individually documented.

Create overload signatures for event-related methods

97f5ac2

The subscribe, unsubscribe, … etc methods are easier to understand (and easier to document) when thought of as two variants: one which accepts one or more event names, and one which doesn’t. So create these overload signatures.

Add this type parameter to EventListener

4979610

Make it clear, via the type system, that a listener can access the name of the emitted event via the `this` variable.

Create type for EventListener’s this

9da57ad

Easier to document.

Create overload signatures for Space.prototype.updateProfileData

cc7d295

Motivation as 97f5ac2.

Rename type param in Subset type

440941f

Elsewhere in the codebase we’ve only used `K` for types related to keys.

Extract return value of Space.prototype.getState to its own type

19ac73f

To make it easier to document.

Fix ably-js links in class-definitions.md

c27c748

I haven’t seen this capitalisation used elsewhere, and I don’t think the fragment is appropriate.

Remove types from class-definitions.md that don’t exist in codebase

f053f80

lawrence-forooghian force-pushed the prepare-for-documentation-comments branch from 20ef7aa to f053f80 Compare October 16, 2023 11:53

lawrence-forooghian merged commit 3fec959 into main Oct 16, 2023
5 checks passed

lawrence-forooghian deleted the prepare-for-documentation-comments branch October 16, 2023 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MMB-156] Prepare for adding documentation comments #197

[MMB-156] Prepare for adding documentation comments #197

lawrence-forooghian commented Sep 28, 2023 •

edited

Loading

dpiatek Oct 6, 2023

lawrence-forooghian Oct 9, 2023

dpiatek Oct 12, 2023

dpiatek Oct 6, 2023

lawrence-forooghian Oct 9, 2023

lawrence-forooghian Oct 9, 2023

dpiatek Oct 6, 2023

lawrence-forooghian Oct 9, 2023

dpiatek Oct 6, 2023

dpiatek left a comment

[MMB-156] Prepare for adding documentation comments #197

[MMB-156] Prepare for adding documentation comments #197

Conversation

lawrence-forooghian commented Sep 28, 2023 • 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

dpiatek left a comment

Choose a reason for hiding this comment

lawrence-forooghian commented Sep 28, 2023 •

edited

Loading