Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Typescript typings. #196

Merged
merged 1 commit into from
May 16, 2018
Merged

Adding Typescript typings. #196

merged 1 commit into from
May 16, 2018

Conversation

saboya
Copy link
Contributor

@saboya saboya commented May 11, 2018

Finishing the Typescript typings spree, following mqttjs/mqtt-packet#39.

Unfortunately aedes needs export assignment, which limits the usage of the typinngs somewhat, but it's still very helpful.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a unit test for the typings? Something that gets transpiled correctly. Something like https://github.com/mqttjs/MQTT.js/tree/master/test/typescript

package.json Outdated
@@ -47,6 +48,7 @@
"websocket-stream": "^5.1.1"
},
"dependencies": {
"@types/node": "^8.10.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer @types/node to be a devDependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, that's my bad.

@saboya
Copy link
Contributor Author

saboya commented May 12, 2018

I adjusted the @types/node dependency and adjusted / added more typings. There are some issues with the typings though:

Like I said, aedes requires export assignment because it exports a function directly. This effectively prevents any other exports, which means that anything defined inside those typings cannot be referenced. This is not an issue for implicit typing, but prevents explicit typing.

Some cases where this might be an issue:

Return Error with returnCode set:

import Aedes = require('aedes')

const authCallback: ,

const Aedes = Aedes({
  authenticate: (client, username: string, password: string, callback) => {
    if (username === 'test' && password === 'test') {
      callback(null, true)
    } else {
      // returnCode is not a property of Error, so this is the
      // only way to fulfill the desired interface from outside
      const error = new Error() as Error & { returnCode: number }
      error.returnCode = 1

      callback(error, false)
    }
  }
})

Indirect assignment of functions that use non-exported definitions:

// Since we cannot reference Client our AuthCallback, we can't define them properly
const authCallback = (client: any, username: string, password: string, callback: any) => {
  if (username === 'test' && password === 'test') {
    callback(null, true)
  } else {
    const error = new Error() as Error & { returnCode: number }
    error.returnCode = 1

    callback(error, false)
  }
}

const Aedes = Aedes({
  authenticate: authCallback
})

Since we cannot re-export packet typings from mqtt-packet like it's done on MQTT.js, usage of those typings require explicit import by the user as well.

Those might or might not be deal-breakers.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m good with not re-exporting the packet types.

types/index.d.ts Outdated
/// <reference types="node" />

import { IPublishPacket, ISubscribePacket, ISubscription, IUnsubscribePacket } from 'mqtt-packet'
import { Socket } from 'net'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be any Duplex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

package.json Outdated
"test": "tape test/*.js test/*/*.js | faucet",
"ci": "npm run lint; npm run coverage",
"ci": "npm run lint; npm run typescript-test; npm run coverage",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use && instead of ;?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I was just sticking to what was already in place :)

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina mcollina merged commit d5f782c into moscajs:master May 16, 2018
@saboya saboya deleted the typescript-typings branch October 21, 2018 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants