[WIP] Add TypeScript 2.1 typings #547

sublimator · 2017-02-20T20:46:53Z

These headers are exported from a branch where I converted the lib/**/*.js to TypeScript. The tests all passed, when I modified them slightly to make sure they referenced package.json main pointing to the tsconfig.compilerOptions.outDir.

sublimator · 2017-02-20T20:50:22Z

re: #536

I extended the client options from Url, because of the funny parsing done, but actually the port is more generally a number so need to fix that.

Also TODO: remove the _\w+ private functions

sublimator · 2017-02-20T20:54:13Z

typings/lib/client.d.ts

+    on(event: 'error', cb: OnErrorCallback): this;
+    on(event: string, cb: Function): any;
+    once(event: 'message', cb: OnMessageCallback): this;
+    once(event: 'packetsend' | 'packetreceive', cb: OnPacketCallback): this;


sublimator · 2017-02-20T20:54:38Z

typings/lib/client.d.ts

+     * _reconnect - implement reconnection
+     * @api privateish
+     */
+    private _reconnect();


need to kill these off

sublimator · 2017-02-20T20:56:02Z

typings/lib/client-options.d.ts

+/**
+ * MQTT CLIENT
+ */
+export interface ClientOptions extends SecureClientOptions, Url {


Should just inline the Url options ... playing around

sublimator · 2017-02-20T21:06:58Z

microsoft/TypeScript#1867 (comment)

Hrmmm

codecov · 2017-03-08T06:11:15Z

Codecov Report

Merging #547 into master will increase coverage by 0.31%.
The diff coverage is 87.71%.

@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
+ Coverage   92.62%   92.93%   +0.31%     
==========================================
  Files           8        8              
  Lines         637      665      +28     
  Branches      151      156       +5     
==========================================
+ Hits          590      618      +28     
  Misses         47       47

Impacted Files	Coverage Δ
lib/connect/tcp.js	`100% <100%> (ø)`	✅
lib/connect/tls.js	`95.45% <100%> (+1.01%)`	✅
lib/connect/ws.js	`80.43% <37.5%> (+3.35%)`	✅
lib/connect/index.js	`95.91% <95.12%> (-1.31%)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d8091d...18f6a81. Read the comment docs.

mcollina

Can we avoid adding those for the private methods?
Also, is there a way to automatically test those?

sublimator · 2017-03-08T09:52:56Z

Removing the private members was my initial impulse also, but one of the m$ devs made a point that it helps when extending the class to not accidentally over-ride private members. The tsc compiler will balk on any attempts to use the members, even on extended classes, where `protected` must be used. Having said that, you could argue it's less a maintenance burden to only expose the public API. Thoughts? I'm easy either way really. Out at the moment so can't make any changes. One other thing to consider is how much of the `url.parse` options do we actually want in the ClientOptions. The 'slashes' etc ... it's kinda messy.

RangerMauve · 2017-03-08T15:12:45Z

With MQTT in the wild, I haven't ever needed to extend it directly, favoring composition over inheritance when adding new capabilities. I would vote for not exposing the private variables for simplicity. (for autocomplete, for example)

sublimator · 2017-03-09T09:56:39Z

So it seems like the consensus is to remove the private variables. There was some discussion about having an option to omit them in the automatically generated .d.ts files (which is what these are) but seems like MS dug their heels in on this one. I'll just remove them manually.

@mcollina As far as tests go, we'd need to have a typescript compiler added to the dev-dependencies, which is not really a biggie. What sort of testing did you have in mind? Just simple tests (connect, subscribe, publish, receive etc) or more extensive ones to somehow notice when the declarations ones get out of sync?

mcollina · 2017-03-09T10:06:56Z

At this point, just making sure if those are valid it's good.

mcollina · 2017-03-09T10:07:08Z

I mean linting them.

sublimator · 2017-03-09T10:22:27Z

Yeah, that seems like a good simple step 👍
Ok, I will remove the private variables, install tslint and add it to the CI build :)

Probably tomorrow

sublimator · 2017-03-10T03:13:43Z

package.json

@@ -84,6 +86,9 @@
    "snazzy": "^6.0.0",
    "standard": "^8.6.0",
    "through2": "^2.0.3",
+    "tslint": "^4.5.1",
+    "tslint-config-standard": "^4.0.0",
+    "typescript": "^2.2.1",


typescript is a peer dependency of tslint

sublimator · 2017-03-10T03:20:06Z

types/lib/client-options.d.ts

+import { QoS } from './types'
+
+export interface IClientOptions extends ISecureClientOptions {
+  // The options are extended by url.parse() fields in connect() function


@mcollina

What do you want exposed here? And once we've decided, should we only extend the IClientOptions with those, rather than
just grabbing the kitchen sink?

> u.parse('mqtt://www.broker.com/mqtt') Url { protocol: 'mqtt:', slashes: true, auth: null, host: 'www.broker.com', port: null, hostname: 'www.broker.com', hash: null, search: null, query: null, pathname: '/mqtt', path: '/mqtt', href: 'mqtt://www.broker.com/mqtt' }

I'll switch to TypeScript branch and find usages and report back

@mcollina I had a crack at cleaning up the url.parse/xtend section. Take a look

sublimator · 2017-03-10T03:22:03Z

types/lib/client.d.ts

+ * (see Connection#connect)
+ */
+export declare class MqttClient extends events.EventEmitter {
+  // public connackTimer: NodeJS.Timer


these weren't prefixed with a _ so I marked them public (default in TypeScript if not specifide) (the first tslint configuration, before I used the standard variant wanted explicit so I just used the --fix options)

In any case, I guess we don't really want them exposed?

sublimator · 2017-03-10T04:11:53Z

lib/connect/index.js

-    opts.clientId = opts.query.clientId
-  }
+  // Someone out in the wild might be using {auth: 'user:pass'} out in the wild
+  // Keep old behaviour? Tests actually pass without this.


Personally I'd just remove this ...

sublimator · 2017-03-10T04:12:52Z

lib/connect/index.js

-    opts = xtend(parsed, opts)
-
-    if (opts.protocol === null) {
+    // Options object always override brokerUrl specified options


I did it this way due to a test:

it('should throw an error when called with no protocol specified - with options', function () { (function () { mqtt.connect('tcp://foo.bar.com', { protocol: null }) }).should.throw('Missing protocol') })

sublimator · 2017-03-10T04:15:36Z

lib/connect/index.js

+      // protocol
+      // clientId
+    {
+      hostname: parsed.hostname,


It's interesting that the servers option uses {host, port} keys:

opts.host = opts.servers[client._reconnectCount].host opts.port = opts.servers[client._reconnectCount].port opts.hostname = opts.host

Perhaps at some point it would be nice just to use one or the other (of hostname/host)

I mean use only one of host or hostname consistently

Hrmmm, ok, so I looked up the url.parse() function and

host is supposedly the hosname + ':' + port

sublimator · 2017-03-10T06:09:29Z

@mcollina

Ok, I removed private variables, and deprecated the hostname option in lieu of host which is used in the servers: [{host: 'localhost', port: 1337},...] option, for consistency. I also removed most of the jibberish url parse options (hash, query, etc), reducing it to just port (as a number) host, path, protocol and clientId and username / password.

sublimator · 2017-03-10T06:13:03Z

The tests on Travis pass, but because of Js code changes related to hostname, the coverage has gone down by a fraction so it's marked as failed.

sublimator · 2017-03-10T06:15:06Z

lib/connect/index.js

+      // protocol
+      // clientId
+    {
+      // hostname: parsed.hostname,


You could argue that to be consistent with deprecating, should leave the hostname until it's completely removed. Assuming you guys are down with that direction.

mcollina · 2017-03-10T08:13:25Z

Can you please do not touch/remove/alter any existing functionality in this PR? I'm not sure what you want to achieve, and that is a semver-major change, while this is not.

I'll comment on the typescript questions later on.

sublimator · 2017-03-10T08:29:33Z

My intention was to clean up the messy extending of the IClientOptions with *ALL* the url.parse variables. It's quite glaring once you see it all mapped out in TypeScript declarations, so I got a bit carried away making alterations, while removing the url jibberish like `hash`/`query` etc. We use hostname/host variously, while in server we use `host`. I added a deprecation warning for `hostname`, for later removal but if removing the rest warrants a sem-ver change then so be it. Is that an issue? I can separate the changes into multiple pull requests if you'd like.

sublimator · 2017-03-10T08:30:16Z

^ while in the *servers* config array we use `host`.

mcollina · 2017-03-10T11:28:08Z

@sublimator are you ok in closing this? Or it's a mistake?

sublimator · 2017-03-10T12:13:25Z

I will open another one later on :) Out at the moment

sublimator commented Feb 20, 2017

View reviewed changes

sublimator mentioned this pull request Feb 28, 2017

Various tweaks: #551

Merged

Add TypeScript declarations

5bc23b5

sublimator force-pushed the typings branch from 6c07e1b to 5bc23b5 Compare March 8, 2017 06:08

sublimator mentioned this pull request Mar 8, 2017

TypeScript implementation? #559

Closed

mcollina requested changes Mar 8, 2017

View reviewed changes

Add typescript dev dependency

501472e

[FOLD ]Add tslint and tslint standard config

95019ec

sublimator commented Mar 10, 2017

View reviewed changes

[FOLD] Clean up url.parse()/xtend options

31b5b73

sublimator commented Mar 10, 2017

View reviewed changes

[FOLD] Deprecate opts.hostname

5282d69

sublimator commented Mar 10, 2017

View reviewed changes

Add deprecation warning

18f6a81

sublimator force-pushed the typings branch from 50230bd to 18f6a81 Compare March 10, 2017 08:10

sublimator closed this Mar 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add TypeScript 2.1 typings #547

[WIP] Add TypeScript 2.1 typings #547

sublimator commented Feb 20, 2017 •

edited

Loading

sublimator commented Feb 20, 2017

sublimator Feb 20, 2017

sublimator Feb 20, 2017

sublimator Feb 20, 2017

sublimator commented Feb 20, 2017

codecov bot commented Mar 8, 2017 •

edited

Loading

mcollina left a comment

sublimator commented Mar 8, 2017 via email

RangerMauve commented Mar 8, 2017

sublimator commented Mar 9, 2017

mcollina commented Mar 9, 2017

mcollina commented Mar 9, 2017

sublimator commented Mar 9, 2017

sublimator Mar 10, 2017

sublimator Mar 10, 2017

sublimator Mar 10, 2017

sublimator Mar 10, 2017

sublimator Mar 10, 2017

sublimator Mar 10, 2017

sublimator Mar 10, 2017 •

edited

Loading

sublimator Mar 10, 2017

sublimator Mar 10, 2017

sublimator commented Mar 10, 2017

sublimator commented Mar 10, 2017

sublimator Mar 10, 2017

mcollina commented Mar 10, 2017

sublimator commented Mar 10, 2017 via email

sublimator commented Mar 10, 2017 via email

mcollina commented Mar 10, 2017

sublimator commented Mar 10, 2017 via email

[WIP] Add TypeScript 2.1 typings #547

[WIP] Add TypeScript 2.1 typings #547

Conversation

sublimator commented Feb 20, 2017 • edited Loading

sublimator commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sublimator commented Feb 20, 2017

codecov bot commented Mar 8, 2017 • edited Loading

Codecov Report

mcollina left a comment

Choose a reason for hiding this comment

sublimator commented Mar 8, 2017 via email

RangerMauve commented Mar 8, 2017

sublimator commented Mar 9, 2017

mcollina commented Mar 9, 2017

mcollina commented Mar 9, 2017

sublimator commented Mar 9, 2017

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

sublimator Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sublimator commented Mar 10, 2017

sublimator commented Mar 10, 2017

Choose a reason for hiding this comment

mcollina commented Mar 10, 2017

sublimator commented Mar 10, 2017 via email

sublimator commented Mar 10, 2017 via email

mcollina commented Mar 10, 2017

sublimator commented Mar 10, 2017 via email

sublimator commented Feb 20, 2017 •

edited

Loading

codecov bot commented Mar 8, 2017 •

edited

Loading

sublimator Mar 10, 2017 •

edited

Loading