chore(types): migrate module resolution and module types #2012

SmashingQuasar · 2025-07-10T11:17:53Z

Description

This PR slightly changes the TypeScript configuration as a first step to migrate towards a stricter TypeScript configuration.

…resolution and module types.

codecov · 2025-07-10T11:19:09Z

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.57%. Comparing base (8f0f4d4) to head (13e1338).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib/connect/tls.ts	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2012      +/-   ##
==========================================
- Coverage   81.68%   81.57%   -0.11%     
==========================================
  Files          25       25              
  Lines        1583     1585       +2     
  Branches      368      368              
==========================================
  Hits         1293     1293              
- Misses        199      201       +2     
  Partials       91       91

Flag	Coverage Δ
unittests	`81.57% <62.50%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

SmashingQuasar

This is a smaller version of my previous PR to migrate to a stricter TypeScript configuration. I added comments to help with the review as always.

@robertsLando could you let me know if there is a way to recognise the browser code and the Node.js code aside from looking at imported dependencies? It would probably make my life a lot easier.

SmashingQuasar · 2025-07-10T11:20:00Z

src/lib/connect/ali.ts

+				if (reader.result instanceof ArrayBuffer) {
+					proxy.push(Buffer.from(reader.result))

-				if (data instanceof ArrayBuffer) data = Buffer.from(data)
-				else data = Buffer.from(data, 'utf8')
-				proxy.push(data)
+					return
+				}
+
+				proxy.push(Buffer.from(reader.result, 'utf-8'))


Since there was a typing issue here, I made a minor refactoring to make the code lighter.

SmashingQuasar · 2025-07-10T11:21:12Z

src/lib/connect/tls.ts

+		// @ts-expect-error - There is a type overlap between readable-stream and stream which are used for browsers and node respectively.
+		// We would need to make it clearer what is aimed at browsers and what is aimed at Node.js to resolve this type issue.


It would be very beneficial to make it clear which files are aimed at browsers and which files are aimed at Node.js within the project architecture. As it is today I find it too risky to change any interface since there are so many overlaps.

SmashingQuasar · 2025-07-10T11:22:08Z

src/lib/shared.ts

@@ -9,7 +10,7 @@ export type GenericCallback<T> = (error?: Error, result?: T) => void

 export type VoidCallback = () => void

-export type IStream = Duplex & {
+export type IStream = (Duplex | NativeDuplex) & {


This change actually represents the reality of how IStream is used. Since this is just a type, it won't affect runtime.
I added @ts-expect-error directives in other places so we don't break existing code.

SmashingQuasar · 2025-07-10T11:22:39Z

tsconfig.json

 	"compilerOptions": {
-		"module": "commonjs",
+		"skipLibCheck": true,


Not added, simply moved up as it will be simpler for me to find which rule is still missing if I preserve the order from my reference.

SmashingQuasar · 2025-07-10T11:23:03Z

tsconfig.json

@@ -1,17 +1,19 @@
 {
+	"$schema": "https://json.schemastore.org/tsconfig",


No other effect than adding autocomplete and error highlighting in the IDE.

robertsLando · 2025-07-10T12:00:52Z

src/lib/connect/ws.ts

+	// @ts-expect-error - This is a type confusion because of the overlap between browser oriented code and Node.js oriented code.
+	// Here we are in a Node.js code since the Ws library explictly says it does not work in the browser.
+	const webSocketStream = Ws.createWebSocketStream(socket, options.wsOptions)


@SmashingQuasar there is no real overlap here, there is a browserStreamBuilder for browsers and this one is for nodejs so I don't understand what you mean here

So the TypeScript error is as follows:

If you look at the signature of Ws.createWebSocketStream(), you will see that it accepts: internal.DuplexOptions as second parameter, which is the DuplexOptions type from node:stream.
On the other hand, we are passing a IClientOptions, which has a wsOptions property with the following typing: ClientOptions | ClientRequestArgs | DuplexOptions.
The problem here is that the DuplexOptions from IClientOptions comes from readable-stream, while the DuplexOptions of Ws.createWebSocketStream() comes from node:stream. So there is a naming overlap between node:stream:DuplexOptions and readable-stream:DuplexOptions.

ClientOptions.wsOptions can be of three different types, we would need to create type guards to narrow down the type and ensure it is indeed a readable-stream:DuplexOptions. I can absolutely implement this, but this will affect runtime and reject any wrongly typed option, which will have possible effects on users. Since we want to have a cautious approach, I elected to use the safest course of action and not touch the code at runtime.

As a small addition to my explanation: From my understanding, the entire point of using readable-stream is to work with browsers, as it is otherwise entirely superseded by the node:stream native library, hence why I am so confused about the mix between node:stream and readable-stream within the code.

ok now I understad what you mean

robertsLando · 2025-07-10T12:05:39Z

tsconfig.json

+		"module": "NodeNext",
+		"moduleResolution": "NodeNext",
+		"target": "ESNext",


For setting og module/moduleResolution and target I think the best is to use @tsconfig/node packages. Maybe in our case https://www.npmjs.com/package/@tsconfig/node20 ?

Yes, I noted that you wanted to extend the configuration, however you also want to introduce as few rules as possible, and the configuration you want to extend introduces "strict": true which will create more errors, and thus more changes.
strict is an alias for many strict rules so it would devolve this PR in a large number of changes. Let me know how you want to proceed.

@SmashingQuasar I would use that and set each strict rule explicit to false and put it true one by one on next PRs

@robertsLando I just did that, the problem is that @tsconfig/node20 is designed to work with Node.js 20, not with a mix of Node.js 20 and browsers API. For example the FileReader class whihc is used in the ali.ts file (which I assume is browser exclusive) can never be resolved.
I'll try to re-extend the correct libraries to avoid breaking more things.

I added a lib: ["DOM"] extension in the tsconfig but honestly, to me this is a really bad idea.
I think you can't reliably extends @tsconfig/node20 until there is a separation between what is for browsers and what is for Node.js or you are taking the risk to use symbols (such as FileReader) that do not exist in one of the environments.

robertsLando · 2025-07-10T14:48:32Z

src/lib/connect/ws.ts

-		options.wsOptions as DuplexOptions,
-	)
+	// @ts-expect-error - This is a type confusion because of the overlap between browser oriented code and Node.js oriented code.
+	// Here we are in a Node.js code since the Ws library explictly says it does not work in the browser.


I think we can remove this comment (and keep only the ts expect error) as it's obvious we are in nodejs env here considering there is also the browser stream builder method

I can remove it for sure.
I just don't find it obvious at all that we are in a Node.js context only within this file, especially since we import isBrowser and have a condition on this on another section of the same file.

SmashingQuasar · 2025-07-10T17:44:23Z

I removed the comment in question.
So for the test coverage, it complains that there are untested lines, but this is because when I change the socksProxy evaluation, it stopped considering this line as tested, because it was only partially tested before.
There does not seem to be a test for the socksProxy option.
I would like to add one so we preserve the coverage, however, I am unsure what the intent would be. Since this is supposed to test a proxy, it would require running a pseudo-proxy to test it. That's doable but what's your policy here?

robertsLando · 2025-07-11T06:52:20Z

@SmashingQuasar We already have tests for socks with a mocked socks server here: https://github.com/mqttjs/MQTT.js/blob/main/test/node/socks.ts

Maybe there is just no test that inits a client with a sock proxy and I think that if that line isn't covered by test it was not even before. Would you like to add a simple test for that?

SmashingQuasar · 2025-07-13T07:42:52Z

@SmashingQuasar We already have tests for socks with a mocked socks server here: https://github.com/mqttjs/MQTT.js/blob/main/test/node/socks.ts

Maybe there is just no test that inits a client with a sock proxy and I think that if that line isn't covered by test it was not even before. Would you like to add a simple test for that?

Hey @robertsLando , sorry for not answering earlier, the end of the week was quite busy.
Indeed I think it would be best to add a test for this case. I am just wondering if we shouldn't wait until we do the architectural changes, since they will most likely affect tests.
Since this line was not tested before, it would be silly to create a test, just to probably completely rewrite it not very long after.
What do you think?

robertsLando · 2025-07-14T07:51:25Z

Since this line was not tested before, it would be silly to create a test, just to probably completely rewrite it not very long after.

It's ok for me 👍🏼

chore: Added a first iteration of TypeScript rules to migrate module …

b987fa4

…resolution and module types.

SmashingQuasar marked this pull request as ready for review July 10, 2025 11:19

SmashingQuasar commented Jul 10, 2025

View reviewed changes

robertsLando requested changes Jul 10, 2025

View reviewed changes

robertsLando changed the title ~~chore: Added a first iteration of TypeScript rules to migrate module resolution and module types.~~ chore(types): migrate module resolution and module types. Jul 10, 2025

SmashingQuasar added 2 commits July 10, 2025 14:06

chore: Extended tsconfig/node20.

e4f8788

chore: Attempted to resolve issues due to @tsconfig/node20 extension.

888ba93

robertsLando changed the title ~~chore(types): migrate module resolution and module types.~~ chore(types): migrate module resolution and module types Jul 10, 2025

robertsLando requested changes Jul 10, 2025

View reviewed changes

chore: Removed a comment.

13e1338

robertsLando approved these changes Jul 14, 2025

View reviewed changes

robertsLando merged commit eeda24e into mqttjs:main Jul 14, 2025
5 of 7 checks passed

SmashingQuasar deleted the dev/nicolas.lebacq/strict-typescript-configuration-01 branch July 14, 2025 12:37

		// @ts-expect-error - There is a type overlap between readable-stream and stream which are used for browsers and node respectively.
		// We would need to make it clearer what is aimed at browsers and what is aimed at Node.js to resolve this type issue.

		@@ -1,17 +1,19 @@
		{
		"$schema": "https://json.schemastore.org/tsconfig",

chore(types): migrate module resolution and module types #2012

chore(types): migrate module resolution and module types #2012

Uh oh!

Conversation

SmashingQuasar commented Jul 10, 2025

Description

Uh oh!

codecov bot commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SmashingQuasar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertsLando Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmashingQuasar Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmashingQuasar commented Jul 10, 2025

Uh oh!

robertsLando commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SmashingQuasar commented Jul 13, 2025

Uh oh!

robertsLando commented Jul 14, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 10, 2025 •

edited

Loading

robertsLando Jul 10, 2025 •

edited

Loading

SmashingQuasar Jul 10, 2025 •

edited

Loading

robertsLando commented Jul 11, 2025 •

edited

Loading