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

Typescript typings #44

Open
HeyJ0e opened this issue Aug 17, 2020 · 12 comments
Open

Typescript typings #44

HeyJ0e opened this issue Aug 17, 2020 · 12 comments

Comments

@HeyJ0e
Copy link

HeyJ0e commented Aug 17, 2020

Is there a definition file for typescript typings?

@HeyJ0e
Copy link
Author

HeyJ0e commented Sep 29, 2020

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 added a commit that referenced this issue Oct 7, 2020
@wankdanker
Copy link
Owner

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

wankdanker added a commit that referenced this issue Oct 7, 2020
@HeyJ0e
Copy link
Author

HeyJ0e commented Oct 7, 2020

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
Copy link

chapterjason commented Oct 20, 2020

Changed a bit:

  • Add exports for the types
  • Update Discover to default export
  • Add me property
  • Add broadcast with instance id (Another issue with easier access to this id would be awesome)

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 added a commit that referenced this issue Oct 20, 2020
@wankdanker
Copy link
Owner

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
Copy link

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

@HeyJ0e
Copy link
Author

HeyJ0e commented Oct 21, 2020

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
Copy link

chapterjason commented Oct 21, 2020

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
Copy link
Author

HeyJ0e commented Oct 21, 2020

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
Copy link

chapterjason commented Oct 21, 2020

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

self.me = {
isMaster : false,
isMasterEligible: self.settings.server, //Only master eligible by default if we are a server
weight : settings.weight,
address : '127.0.0.1', //TODO: get the real local address?
advertisement : options.advertisement
};

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:

console.log('instanceUuid', d.broadcast.instanceUuid);

@chapterjason
Copy link

chapterjason commented Nov 24, 2020

Hey @wankdanker and @HeyJ0e

several days ago I refactored the whole project.

Highlights:

  • Full TypeScript support (Full (re)written in TypeScript)

    • Define advertisement and channel and message data format
      Screenshot 2020-11-24 at 6 28 48 PM

      Screenshot 2020-11-24 at 6 31 08 PM
    • Typed events
      Screenshot 2020-11-24 at 6 32 09 PM

  • Code documentation

    Screenshot 2020-11-24 at 6 33 41 PM
  • Documentation generation with typedoc

  • Decoupled leadership election

  • Decoupled different network types (unicast, multicast, broadcast)

  • Latest supported version to node 10

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
Copy link
Owner

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.

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

No branches or pull requests

3 participants