Refactor the leadership module to make use of existing and new events

[wankdanker]

It is not totally necessary to create a whole new class/interface to manage the leadership. With the addition of a few new events, it's possible to wire everything up just using events. I feel like this makes it more generic as well.

This commit removes most of the internal references to leadershipElector and modifies the BasicLeadershipElection class to accept the node-discover instance in the constructor. The constructor then wires all the events to the class methods.

This restores the expected behavior that is reported in #36

ref: #36, #35, #34

@aikar Would this still work with what you needed?

[aikar]

I originally considered going this route too, but ended up never doing it but agree it is a good thing.

However I wanted to use start as a hook point to say "intercept starting, allowing preparing the instance before continuing".

For example, I hooked in my own extra properties on discover.me before starting so the first payload can have it.

My concern is that the discover instance auto starts, and I wanted a way to control that.

I guess alternatively, autostart:false as an option and allow calling .start() manually?

[aikar]

Also I went with the original start route and pass my own instance as I wanted to use dependency injection on my leadership elector instance, so that it has references to my own systems objects, which timing wise is difficult with the current auto start.

atm I have leadershipElector: new MyLeadershipElector(this) then set this.discover in .start() so I have access to both guaranteed at any event fire.

[aikar]

Could solve that by moving the event binders to the start method and keep it like I originally had it with constructor-less invocation.

Doing it that way I think also avoids making this an API breaking change (considering the new .start() API is now released and I'm using it in my own code, plus passing an instantiated object vs the constructor in options)

[wankdanker]

What if there was just a way to turn off the internal promotion/demotion code (which is now a module)? That in addition to the start : false option, you could just instantiate it and then wire up all the events to do whatever you want. I think that would have been the ideal way to go.

But, since the API has already been released, I can add some extra logic depending on the value of opts.leadershipElector.

• if opts.leadershipElector is a function
  • instantiate it with self as the constructor
  • bind events to methods
  • also call start method passing in self?
• if opts.leadershipElector is an object
  • bind events to methods
  • call start method passing in self
• if opts.leadershipElector is null or undefined
  • use the backwards compatible leadership module
• if opts.leadershipElector === false
  • don't do any leadership stuff

[aikar]

Left some feedback but I think overall this is good w/ me now. the .start change is technically an API breaking change as it changes existing behavior (and I will have to update code to account for that) but I'm ok with that since the start option was added.

If you want to skip bumping semver major, I would be ok with that as i'm pretty confident i'm the only one this would impact and i'll resolve it on next update...

[wankdanker]

If you want to skip bumping semver major, I would be ok with that as i'm pretty confident i'm the only one this would impact and i'll resolve it on next update...

Cool, thank you. I'll merge this and release 1.1.2.

[wankdanker]

node-discover@1.1.2 is on npm.