wankdanker / node-discover

Automatic and decentralized discovery and monitoring of nodejs instances with built in support for a variable number of master processes, service advertising and channel messaging.
229 stars 59 forks source link

Typescript typings #44

Open HeyJ0e opened 4 years ago

HeyJ0e commented 4 years ago

Is there a definition file for typescript typings?

HeyJ0e commented 3 years ago

I've created a version:

declare module 'node-discover' {
  import { EventEmitter } from 'events';
  import { RemoteInfo } from 'dgram';

  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;
  };

  type Message<T=unknown> = {
    event: string;
    pid: string;
    iid: string;
    hostName: string;
    data?: T;
  };

  type Node<A=unknown> = {
    isMaster: boolean;
    isMasterEligible: boolean;
    weight: number;
    address: string;
    lastSeen: number;
    hostName: string;
    port: number;
    id: string;
    advertisement?: A;
  };

  type ThisNode<A=unknown> = {
    isMaster: boolean;
    isMasterEligible: boolean;
    weight: number;
    address: string;
    advertisement?: A;
  };

  type ChannelListener<D> = (data: D, obj: Message<D>, rinfo: RemoteInfo) => void;

  class Discover<A=any, C extends Record<string, any>=Record<string, any>> extends EventEmitter {
    nodes: Record<string, Node>;
    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;
  }

  export = Discover;
}

I've made it generic, so you can define the advertisement and channel data:

type Advertisement = {
  prop1: number;
  prop2: string;
};

type Channels = {
  channel1: number;
  channel2: string;
  channel3: {
    prop: boolean;
  };
};

const d = new Discover<Advertisement, Channels>();
wankdanker commented 3 years ago

I added your code as index.d.ts here. Would that accomplish what you need?

HeyJ0e commented 3 years ago

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.

chapterjason commented 3 years ago

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;
    }
}
wankdanker commented 3 years ago

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!

chapterjason commented 3 years ago

I think the most is covered, but if you can wait several days I will take a look and will improved it.

HeyJ0e commented 3 years ago

Making exports is a good idea, but I think exposing the me and the broadcast property is not that so. This would make the false impression that you can modify them. It would be better to have getters (as you mentioned) or at least mark as readonly.

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,

chapterjason commented 3 years ago

Making exports is a good idea, but I think exposing the me and the broadcast property is not that so. This would make the false impression that you can modify them. It would be better to have getters (as you mentioned) or at least mark as readonly.

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.

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,

I don't know if there are any policies, I think it's another issue to introduce something like eslint.

HeyJ0e commented 3 years ago

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:

readonly me: Readonly<ThisNode<A>>;
chapterjason commented 3 years ago

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 https://github.com/wankdanker/node-discover/blob/347dd578632250b89d7730ce079b2cbd52ff408f/lib/discover.js#L177-L183

console.log objects

const Discover = require("node-discover");

const d = new Discover({}, () => {
    console.log(d);
});

image

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 image

It is possible to do this. So I would defer this and change the source first.

//EDIT Even in the examples: https://github.com/wankdanker/node-discover/blob/347dd578632250b89d7730ce079b2cbd52ff408f/examples/basic-argv.js#L20

chapterjason commented 3 years ago

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. It is also not good to have dev dependencies for "better" examples, it is just a bad DX.

Take a look in the fork under the branch project-refactor: https://github.com/chapterjason/node-discover/tree/project-refactor

wankdanker commented 3 years ago

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.