feat: strict TypeScript configuration

[SmashingQuasar]

Description

Following the discussion from #2006, this PR aims to implement a strict TypeScript configuration.

[SmashingQuasar]

@robertsLando : I already resolved many issues but this is taking some time to fix so I will continue in the upcoming days.

There are a few cases where I elected to put an @ts-expect-error directive to tell TypeScript that it should find an error there but ignore it.
Sometimes, I added an @TODO with some ideas for improvement. I also mentioned you a few times (since I don't know who else can be involved in this). We can remove those @TODO before merging, I just wanted to draw your attention on those.

[SmashingQuasar]

I added a few comments just to give more insight on what I did so far.

[SmashingQuasar]

Not strictly necessary but enables migration to ESM.

[SmashingQuasar]

Best practice to avoid confusion between folders and files, there are also other more specific requirements from Node.js and browsers if using .mjs extension.

[SmashingQuasar]

All of these, and every access made that way can be easily resolved by using typeguards in a later PR.

[SmashingQuasar]

I elected to create this very simple interface to limit cognitive load on function signature. It also simplified later typing.

[SmashingQuasar]

The override keyword is mandatory here to indicate we are overriding the behaviour of a method that was already defined in the parent class. This improves maintainability.

[SmashingQuasar]

Not strictly a TypeScript issue, but ESlint will complain if we use a strict configuration, as nullish comparisons can lead to problematic bugs.

[DavideViolante]

what if timerId is null?

[SmashingQuasar]

timerId cannot be null in this code.

It is declared line 9 as:

	private timerId: number | undefined

It is then used from lines 50 to 55 here:

	private clear() {
		if (this.timerId !== undefined) {
			this.timer.clear(this.timerId)
			this.timerId = undefined
		}
	}

It's last reference is line 101 with:

		this.timerId = this.timer.set(() => {
			// this should never happen, but just in case
			if (this.destroyed) {
				return
			}

			this.counter += 1

			// after keepalive seconds, send a pingreq
			if (this.counter === 2) {
				this.client.sendPing()
			} else if (this.counter > 2) {
				this.client.onKeepaliveTimeout()
			}
		}, this._intervalEvery)

The Timer.prototype.set method can only return a number:

This means timerId can only be either a number or undefined.

[SmashingQuasar]

A dangerous code that was revealed by using a strict TypeScript configuration. You have two very clear cases where those properties can unexpectedly end up as NaN.

[SmashingQuasar]

Here again a strict TypeScript configuration reveals a potential error that will be uncaught and thus bubble back to our end users code. I left this new Error('') blank for now since I still need to understand better what to do in this situation. I'm not completely familiar with this code base.

[robertsLando]

I'm not completely familiar with this code base.

The problem with this is I never had a way to test this out, the user using this are very few and sincerely I dunno if it works or not. Same for wx, when I did the TS refactor I just focused on converting it to TS without worry too much about it

[robertsLando]

There was a plan BTW to drop them and put them on a separated repo, same for pub/sub bin but giving my limited time I'm just focusing on fixing bugs for now

[SmashingQuasar]

No worries, every code base has problems, even if you have 100% coverage, 100% stryker score and 0 SonarQube issue you have problems that are not clearly identified. I'm just highlighting the potential benefits of a stricter configuration.
This is actually really positive as we can improve this part since we know it has a potential issue here. 👍

[robertsLando]

I think we could drop minimis in favor of nodejs parseArgs but maybe we can do that in another PR.

[SmashingQuasar]

Yes, just make sure that this does not break browsers implementation. I think there is a global dependency review to be done on the overall code. Let's add this to a roadmap if there is one. 👍

[robertsLando]

just make sure that this does not break browsers implementation

Minimist isn't used on browser side but just on /bin files that can only be run from nodejs :)

[robertsLando]

could this throw if so?

[robertsLando]

I'm wondering if using 0 instead of null here may lead to some errors elsewhere? I would stick to undefined and fix the type here

[SmashingQuasar]

Good question. This section of the code is treacherous. I think the best course of action would be to refactor it so it is simpler to understand. I am trying to limit the changes I am making to the bare minimum to have a strict TypeScript configuration so the review is easier. I'll see what I can do make this more stable.

[robertsLando]

never called? It's actually used AFAIK

[SmashingQuasar]

For some reason, VSCode failed to detect the call within auth.ts.

After having a look, here is the full method declaration:

	/**
	 * @param packet the packet received by the broker
	 * @return the auth packet to be returned to the broker
	 * @api public
	 */
	// @ts-expect-error - packet is unused. @TODO: This method is seemingly never called.
	public handleAuth(packet: IAuthPacket, callback: PacketCallback) {
		callback()
	}

The only place I find it used is here:

	client.handleAuth(
		packet,
		(err: ErrorWithReasonCode, packet2: IAuthPacket) => {
			if (err) {
				client.emit('error', err)
				return
			}

			if (rc === 24) {
				client.reconnecting = false
				client['_sendPacket'](packet2)
			} else {
				const error = new ErrorWithReasonCode(
					`Connection refused: ${ReasonCodes[rc]}`,
					rc,
				)
				client.emit('error', error)
			}
		},
	)

If we look at the code, we can see that both arguments err and packet2 will both be undefined as the handleAuth implementation passes no argument to the callback argument invocation.

From what I can see, the code that is really being ran is:

			if (rc === 24) {
				client.reconnecting = false
				client['_sendPacket'](undefined) // packet2 is undefined so I replaced it by its constant value
			} else {
				const error = new ErrorWithReasonCode(
					`Connection refused: ${ReasonCodes[rc]}`,
					rc,
				)
				client.emit('error', error)
			}

I haven't had a look at auth.ts so I can't tell if this is intended.

[robertsLando]

This is never used but can be overridden by user in order to handle messages with backpressure on stream (instead of using message event that doesn't provide backpressure)

[SmashingQuasar]

Alright, I don't have the entire code in mind, I'll remove the @TODO here since I can't really tell how an end-user would use this.

[robertsLando]

I know, there was a PR on mqtt-packet to fix this type don't remember right now if it has been merged or not but it was safe to force it to buffer especially because almost everyone passes it as string

[robertsLando]

clientId should never be undefined, we generate a random one in case it is

[robertsLando]

MQTT.js/src/lib/client.ts

Lines 576 to 579 in 1720b64

	this.options.clientId =
	typeof options.clientId === 'string'
	? options.clientId
	: MqttClient.defaultId()

[SmashingQuasar]

I agree, but in that case, the corresponding interface IClientOptions should reflect that and forbid the property from being undefined.

[robertsLando]

Why not string[] ?

[SmashingQuasar]

That's a habit that I have, it's not even conscious at this point.

There is actually a good reason to this.
In TypeScript, [] can be used to defined two very different but related structures.
string[] is equivalent to Array<string>. This means an array of length 0 or more.
However, you can also use TypeScript tuples, which are specific arrays, for example:
[string, ...Array<string>] which means : "An array of strings with a minimum length of 1".

I've seen many developers quickly get confused when getting this kind of typing (I was even slightly confused while typing it):

const parsedValue: [[string, ...string[]], [string, ...string[]]];

I usually write:

const parsedValue: [[string, ...Array<string>], [string, ...Array<string]];

This helps with readability and isolates [] use to tuples only to reduce cognitive load.
Sub-typed structures also have a common structure in TypeScript:

const bar: Record<string, unknown> = {};

class Foo<T> {
   public setSomething(value: string): void {}
}

const myFoo: Foo<string> = new Foo<string>();

type FooSetSomething = Parameters<Foo<string>['setSomething']>;

const myMap: Map<string, string> = new Map<string, string>();

I find it more consistent to respect this syntax for every structure instead of creating an exception for arrays, especially if you end up with this kind of type:

const multiTypesArray: (number|string|string[])[] = [];

This is just an opinion. If you want to switch to string[] just let me know, it's not a problem.

[robertsLando]

why this has been removed?

[SmashingQuasar]

This code was set here, and then immediately reaffected under the similar condition here:

				if (version === 5) {
					currentOpts.nl = reconciledOptions.nl ?? false
					currentOpts.rap = reconciledOptions.rap ?? false
					currentOpts.rh = reconciledOptions.rh ?? 0
					// use opts.properties
					currentOpts.properties = properties
				}

So I simply moved it in one place. This resolved some typing issues since the interface indicates that nl, rap and rh can be undefined.

[robertsLando]

Dunno why I didn't do it here but I usually extend node default tsconfig here, like https://www.npmjs.com/package/@tsconfig/node20.

[SmashingQuasar]

Up to you if you want to use a base, I'm providing the strictest configuration I currently am aware of.

[DavideViolante]

Starting with the strictest config is not a smart idea imho, it will bring too many changes in a single PR. I'd proceed way more cautious since this library is literally used by millions.

[SmashingQuasar]

I understand the concerns. Then it might be impossible to switch to a strict ESlint configuration because there are hundreds of rules, which would take extremely long to implement rule by rule.

I'll start over with one rule at a time...

[DavideViolante]

I'd proceed more carefully on this and on a step by step basis. Maybe initially just adding "noImplicitAny" rule and merging less changes, then adding stricter properties like strict step by step in different PRs. Also I'd change the "module" in a different PR. Just my thoughts. This lib is used by millions, this PR imho is waaaaay to big.

[SmashingQuasar]

I'd proceed more carefully on this and on a step by step basis. Maybe initially just adding strict to tsconfig and merging less changes, then adding stricter properties step by step in different PRs. Also I'd change the "module" in a different PR. Just my thoughts.

We could do it that way if that makes people more comfortable. Let me know how we shall proceed and I will adapt the process accordingly.

[SmashingQuasar]

I just pushed a commit with many new issues fixed. I haven't cleaned up the previously discussed one for now, I'll do a complete clean up at the end to avoid having to go back and forth. 225 errors left to resolve.

[SmashingQuasar]

I resolved many typing issues again and added some comments to help with the review.

[SmashingQuasar]

Explicit but discreet cast to number.

[SmashingQuasar]

The syntax:

typeof x === 'undefined'

Is no longer useful as ECMAScript made the undefined global symbol read-only many years ago. This was a necessity for everything prior ES5.

[SmashingQuasar]

So here, what I think is happening, is that we want to use the same instantiation interface and definition interface, which creates these situations where we constantly have to check if anything is undefined.

[SmashingQuasar]

Very likely that this code is never ran as storePacket ends up being the cloning function and not the cloned packet. clone() returns the cloning function so the correct syntax would be clone()(packet). This is because the developers of this function used a purely functional approach. (which is not inherently bad but requires care when used to avoid this kind of situation)

[robertsLando]

I'm not sure. The export of default is:

module.exports = require('./index.js')()

So I think this is actually working

[SmashingQuasar]

This method is definitely public, as it is called from other files.

[robertsLando]

same for _onConnect... dunno what's best to do

[SmashingQuasar]

So I think that we should refactor all properties to actually use explicit (and not magic!) getters to guarantee types and avoid the perpetual pain of having possibly undefined values everywhere. It would considerably reduce the size of the code as well.

[SmashingQuasar]

Definitely public as it is called from other files.

[robertsLando]

yeah kind of, in fact we don't really want this to be called by users but we need to call it in our handlers, they used to be inside client.ts but I moved them away as I would like to split it as there was too much code inside that single class

[SmashingQuasar]

RegExp.prototype.exec is the recommended way to use regular expressions nowadays. I also converted this to use named groups to make the intent clear.

[SmashingQuasar]

I really like this example in fact as it clearly highlights why I always ban null in all code base that I work with except for JSON serialisation.
The following code:

console.debug(typeof null === 'object');

Will output true.

This is because null is not actually null. This means that it is indeed an object.
The two programming concepts you usually want to use is null value which exists only in the form:

const myValue: unknown = undefined; // This is a true reference to null value

And null reference, which can be observed that way:

interface ExampleInterface
{
  possibleNullReference?: string;
}

const example1: ExampleInterface = {
  possibleNullReference: "foo"
}

console.debug(Object.hasOwn(example1, "possibleNullReference")); // true

const example2: ExampleInterface = {};

console.debug(Object.hasOwn(example2, "possibleNullReference")); // false

Which is also why those two syntax:

interface ExampleInterface {
  foo?: string
}

and

interface ExampleInterface {
  foo: string | undefined
}

are not equivalent.

Additionally, you can even find even weirder situations such as:

const foo = Object.create(null);

foo.bar = "This property exists";

console.debug(typeof foo); // "object" so Javascript clearly identifies this as an object type
console.debug(foo instanceof Object); // false because this object does not have the Object prototype, it's a void-prototype object
console.debug(foo.bar); // "This property exists" => It is still a functioning object

[robertsLando]

Yep I know that and I agree about having more strict types for this reason

[SmashingQuasar]

So, require is actually a rather simple module import implementation based on a capsule.
A CJS module actually is something like:

((__dirname, __filename, module) => {
  // Your CJS code
})()

The problem is that CJS is mutable because it is not built-in Javascript. So if you were to do this:

global.require = (path) => {
  const result = require(path);

  Object.keys(require.cache).forEach((key) => {
    delete require.cache[key]
  })

  return result
}

Then the require cache would cease to exist, which will result in an extreme amount of IOPS when constantly calling require.

[robertsLando]

considering that this happens only on client creation I don't think it's a real bottle neck for performances BTW I'm open for improvements :)

[robertsLando]

BTW using 127.0.0.1 here may cause other compatibility issues I think?

[robertsLando]

see my comment

[robertsLando]

what about assing noop to done when not defined? This would remove some useless code

[robertsLando]

@SmashingQuasar Giving the growing amount of changes this PR is introducing I agree with @DavideViolante that a better and safer approach could be to split this into smaller PR where in each one you enable 1/2 strict rules of TS till we reach our goal.

[SmashingQuasar]

@SmashingQuasar Giving the growing amount of changes this PR is introducing I agree with @DavideViolante that a better and safer approach could be to split this into smaller PR where in each one you enable 1/2 strict rules of TS till we reach our goal.

Alright, though this is getting very painful for me as I will have to start over. I'll do it with the minimal number of rules possible to avoid having to redo the entire work again.

[SmashingQuasar]

Closing this PR to reopen it one rule at a time.