Improve Typescript typings to allow explicit typing #206
Conversation
|
I don't think that my changes are cause of Travis build failing. It failed only on node 10 on some timeout related tests and I haven't touched the core logic at all. |
test/typescript/typings.ts
Outdated
| @@ -1,24 +1,25 @@ | |||
| // relative path uses package.json {"types":"types/index.d.ts", ...} | |||
|
|
|||
| import Aedes = require ('../..') | |||
| import aedes = require ('../..') | |||
| import { Client, AuthenticateError } from '../..' | |||
There was a problem hiding this comment.
I think we should really expose the server as aedes.Server.
Basically we should add to aedes.js:
aedes.Server = aedes
In this way you can do:
import { Server, Client, AuthenticateError } from 'aedes'This should not be a semver-major change, but just a facility for those using ESM and Typescript.
There was a problem hiding this comment.
@mcollina what do you think about using default property instead?
Aedes.default = AedesThis way the main function could be imported using default import syntax, while still exposing types if needed
import aedes, { Client, AuthenticateError } from 'aedes'
// or Server, whatever default export name someone preferIn fact, aedes can already be used this way with Babel or even with Typescript with esModuleInterop setting enabled. Unfortunately, by default TS module loader expects default property to be present.
There was a problem hiding this comment.
Unfortunately, from what I've checked there is no way to change typings in a way that would allow both - current require syntax and default import in Typescript. This means that there would be a breaking change for people that currently use aedes in TS without esModuleInterop enabled.
I still think that default import is the way to go, especially for discoverability - when looking at JS examples, I think it's more obvious that const aedes = require('aedes') translates to import aedes from 'aedes' than to import { Server } from 'aedes', especially since with Babel a default import already works just fine.
If you don't want to introduce breaking changes or don't like default import syntax, I'll change it the way you suggested.
There was a problem hiding this comment.
I would prefer the way I suggested, as it’s an additive change and it does not require a change in ts configuration.
There was a problem hiding this comment.
I might have not explained it clearly - it wouldn't require ts configuration change, but change in the way it's currently imported. In any case, I added the Server property as you suggested.
|
Is this PR waiting on something? Just readme update? I just switched from Mosca to Aedes and I find the TS definition currently released leaves a lot to be desired... this PR would do it a lot of good! I am happy to help with TS side of things. |
|
I'll do the quick readme update. Feel free to send more TS prs! |
|
@mcollina thanks muchly for merging the PR... I'll update my fork (which seems to be failing a test) and then have a quick run around the TS bindings again. |
With current type definitions it's not possible to use explicit typing. I changed them based on this template to allow to import all defined types.
Besides that, I added an
AuthenticateErrortype and changed typescript tests to use explicit typing in a few places. I also changed some names in tests file to avoid naming collisions and to be more in line with other tests.Not being able to use explicit typing is a problem in my project, so I hope you will find these changes useful.