-
Notifications
You must be signed in to change notification settings - Fork 59
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
Typescript typings #44
Comments
I've created a version:
I've made it generic, so you can define the advertisement and channel data:
|
I added your code as |
Mostly yes. I think it's worth mentioning in the readme with an example (like the one I provided). Did you check the definitions? I'd be happy if someone could verify them. |
Changed a bit:
In long term I would recommend switching to typescript, it's a lot easier to maintain. If you need any help just ask. declare module "node-discover" {
import { EventEmitter } from "events";
import { RemoteInfo } from "dgram";
export type Options = {
helloInterval?: number;
checkInterval?: number;
nodeTimeout?: number;
masterTimeout?: number;
address?: string;
port?: number;
broadcast?: string;
multicast?: string;
multicastTTL?: number;
unicast?: string | string[];
key?: string;
mastersRequired?: number;
weight?: number;
client?: boolean;
server?: boolean;
reuseAddr?: boolean;
exclusive?: boolean;
ignoreProcess?: boolean;
ignoreInstance?: boolean;
start?: boolean;
hostname?: string;
};
export type Message<T = unknown> = {
event: string;
pid: string;
iid: string;
hostName: string;
data?: T;
};
export type Node<A = unknown> = {
isMaster: boolean;
isMasterEligible: boolean;
weight: number;
address: string;
lastSeen: number;
hostName: string;
port: number;
id: string;
advertisement?: A;
};
export type ThisNode<A = unknown> = {
isMaster: boolean;
isMasterEligible: boolean;
weight: number;
address: string;
advertisement?: A;
};
export type ChannelListener<D> = (data: D, obj: Message<D>, rinfo: RemoteInfo) => void;
export type Network = {
instanceUuid: string;
};
export default class Discover<A = any, C extends Record<string, any> = Record<string, any>> extends EventEmitter {
nodes: Record<string, Node>;
me: ThisNode<A>;
broadcast: Network;
constructor(callback?: (error: Error, something: boolean) => void);
constructor(options?: Options, callback?: (error: Error, success: boolean) => void);
promote(): void;
demote(permanent: boolean): void;
join<T extends keyof C>(channel: T, cb: ChannelListener<C[T]>): boolean;
leave<T extends keyof C>(channel: T): boolean;
advertise(advertisement: A): void;
send<T extends keyof C>(channel: T, obj: C[T]): boolean;
start(callback?: (error: Error, success: boolean) => void): false | boolean;
stop(): false | boolean;
eachNode(fn: (node: Node<A>) => void): void;
on(event: "promotion", listener: (me: ThisNode<A>) => void): this;
on(event: "demotion", listener: (me: ThisNode<A>) => void): this;
on(event: "added", listener: ChannelListener<Node<A>>): this;
on(event: "removed", listener: (node: Node<A>) => void): this;
on(event: "master", listener: ChannelListener<Node<A>>): this;
on(event: "helloReceived", listener: (node: Node<A>, obj: Message<Node<A>>, rinfo: RemoteInfo, isNew: boolean, wasMaster: null | boolean) => void): this;
on(event: "helloEmitted", listener: () => void): this;
on(event: "error", listener: (error: Error) => void): this;
on(event: "check", listener: () => void): this;
on(event: "started", listener: (self: Discover) => void): this;
on(event: "stopped", listener: (self: Discover) => void): this;
on(channel: "hello", listener: ChannelListener<A>): this;
on<T extends keyof C>(channel: T, listener: ChannelListener<C[T]>): this;
}
} |
Thanks, @chapterjason. I've updated https://github.com/wankdanker/node-discover/tree/issue-44-index.d.ts, if it looks good to everyone, including whatever is necessary in package.json (or elsewhere) to tie it all together, lmk. I can merge it and release. Cheers! |
I think the most is covered, but if you can wait several days I will take a look and will improved it. |
Making exports is a good idea, but I think exposing the Plus a minor thing: quotes. I don't know what's the policy for this project, in the code I saw both single and double, |
If you take a look in the source they are accessible even if there are types or not, that shouldn't be an issue for the types. This should be changed in source first. There are even more things that are accessible and not in the proposed types.
I don't know if there are any policies, I think it's another issue to introduce something like eslint. |
Yes, there's more things accessible, but that's true for a lot of javascript projects. I think we shouldn't encourage this behaviour with the types but nudge users to the 'right direction'. Let's say like this:
|
I totally agree to guide the user. But I would change this in the source first, cause this will only apply for typescript users. In all other cases (except docs) like: a look in the source Lines 177 to 183 in 347dd57
console.log objects const Discover = require("node-discover");
const d = new Discover({}, () => {
console.log(d);
}); just override properties const Discover = require("node-discover");
const d = new Discover({}, () => {
d.broadcast.instanceUuid = 'bar';
console.log(d.broadcast.instanceUuid);
}); $ node index.js
bar a good IDE shows you the properties It is possible to do this. So I would defer this and change the source first. //EDIT node-discover/examples/basic-argv.js Line 20 in 347dd57
|
Hey @wankdanker and @HeyJ0e several days ago I refactored the whole project. Highlights:
I think several things could be improved, but in first place it's a good start. The examples should be changed to typescript too. Take a look in the fork under the branch project-refactor: |
Hey @chapterjason. It looks pretty nice. Would you be willing to take over maintainership of the project if this gets merged? I don't have any time to help maintain a code base that I'm familiar with, let alone a complete rewrite in another language. |
Is there a definition file for typescript typings?
The text was updated successfully, but these errors were encountered: