node-ads icon indicating copy to clipboard operation
node-ads copied to clipboard

Promise support

Open roccomuso opened this issue 7 years ago • 12 comments

  • [x] Support for Promises.

roccomuso avatar Aug 08 '18 12:08 roccomuso

Promises? How classic classic ^^ I would recommend the use of observables with RxJs http://reactivex.io/

ovi1337 avatar Aug 08 '18 13:08 ovi1337

Your question was regarding promises right?

ovi1337 avatar Aug 08 '18 13:08 ovi1337

We could discuss it.

  • They're easier to use, with a common and modern interface.
  • Right now we're tailored to the "function" keyword for the callback local scope, limiting the use of arrow function too, this leads easily to scope's errors.

@ovi1337 I don't see any benefit in using it instead of a simple EventEmitter.

roccomuso avatar Aug 08 '18 13:08 roccomuso

There are many benefits, you can handle subscriptions, unsubscriptions, debounces, filters, merges, you can collect and share events and many many more things. We're using it since a long time in our company and is very solid. Angular and other large frameworks are also using it and it's epic and the best and most comfortable event handling what you currently can have. We can completely get rid of the crappy callbacks. And working with events is much more clear and is much easier to develop.

ovi1337 avatar Aug 08 '18 13:08 ovi1337

No doubts about it, but it's a kinda big framework and I feel like we would introduce too much complexity. We could achieve the same with a simpler eventemitter2 maybe?

roccomuso avatar Aug 08 '18 13:08 roccomuso

RxJs is completely tree-shakable - it means that only the used functions are injected into the final build, if you're using Webpack, FuseBox or other build tools. It's maybe possible to use the other one, but it's less supported and less known. In total it's not really smaller. ReactiveX is supported and improved by Microsoft, Soundcloud, AirBnB, Google, GitHub, Netflix and other global players. There are many many contributors and a managed team behind. This is a huge benefit...

But in general i totally agree @ChrisHanuta.

In fact of lightweight is the use of Promises much more comfortable instead of the callback hell.

ovi1337 avatar Aug 08 '18 13:08 ovi1337

Yeah but this would require also a bigger dev pipeline to support older versions of node etc. I agree to keep it as slim as possible with a few numbers of dependencies. Supporting Promise wouldn't be too hard to do. The idea could be to keep both callbacks and Promise.

roccomuso avatar Aug 08 '18 14:08 roccomuso

Hi Guys Thanks for this ads library, I'm waiting for promise support to use this lib. Is there any update about it? Thanks again.

ekuzu avatar Aug 28 '18 15:08 ekuzu

Promises and Typescript are kind of industry standard and would be very nice to have ✌️

dionysiusmarquis avatar Aug 29 '19 11:08 dionysiusmarquis

I'm ok with promises. Typescript would mean a complete rewrite.

roccomuso avatar Aug 29 '19 11:08 roccomuso

I'm on a rewrite in typescript and with necessary definitions to use it with comfort, but currently i don't have much time to finish it. I've also rewritten it with a better structure and class typology.

ovi1337 avatar Aug 29 '19 11:08 ovi1337

@roccomuso It is also possible to add a .d.ts to declare the types I started to define everything I'm using so far. It looks like this atm:

export enum AdsTypeLength {
  BOOL = 1,
  BYTE = 1,
  WORD = 2,
  DWORD = 4,
  SINT = 1,
  USINT = 1,
  INT = 2,
  UINT = 2,
  DINT = 4,
  UDINT = 4,
  LINT = 8,
  ULINT = 8,
  REAL = 4,
  LREAL = 8,
  TIME = 4,
  TIME_OF_DAY = 4,
  TOD = 4, // TIME_OF_DAY alias
  DATE = 4,
  DATE_AND_TIME = 4,
  DT = 4, // DATE_AND_TIME alias
  STRING = 81
}

export enum AdsNotify {
  CYCLIC = 3,
  ONCHANGE = 4
}

export interface AdsOptions {
  host: string
  // The NetId of the target machine
  amsNetIdTarget: string
  // The NetId of the source machine.
  // You can choose anything in the form of x.x.x.x.x.x,
  // but on the target machine this must be added as a route.
  amsNetIdSource: string

  // OPTIONAL: (These are set by default)
  // The tcp destination port
  port?: number | string
  // The ams source port
  amsPortSource?: number | string
  // The ams target port for TwinCat Runtime
  amsPortTarget?: number | string
  // The timeout for PLC requests
  timeout?: number
}

interface AdsHandleInterface {
  // Handle name in twincat
  symname: string
  // An ads type object or an array of type objects.
  // You can also specify a number or an array of numbers,
  // the result will then be a buffer object.
  // If not defined, the default will be BOOL.
  bytelength?: AdsTypeLength | AdsTypeLength[]
  // The propery name where the value should be written.
  // This can be an array with the same length as the array length of byteLength.
  // If not defined, the default will be 'value'.
  propname?: string | string[]
}

interface AdsNotificationHandleInterface extends AdsHandleInterface {
  // OPTIONAL: (These are set by default)
  // Notify.ONCHANGE, (other option is Notify.CYCLIC)
  transmissionMode?: AdsNotify
  // Latest time (in ms) after which the event has finished
  maxDelay?: number
  // Time (in ms) after which the PLC server checks whether the variable has changed
  cycleTime?: number
}

type AdsHandleWithValues<H extends AdsHandleInterface, V extends {}> = H & { [value in keyof V]?: V[value] }

export type AdsHandle<V = {value}> = AdsHandleWithValues<AdsHandleInterface, V>
export type AdsNotificationHandle<V = {value}> = AdsHandleWithValues<AdsNotificationHandleInterface, V>

enum AdsClientEvent {
  Error = 'error',
  Notification = 'notification'
}

interface AdsClientEventDataMap {
  [AdsClientEvent.Error]: Error,
  [AdsClientEvent.Notification]: AdsNotificationHandle
}

interface AdsClient {
  connect (options: AdsOptions, cb: () => void): void
  end (): void
  read (handle: AdsHandle, cb: (error: Error, handle: AdsHandle) => void): void
  write (handle: AdsHandle, cb: (error: Error) => void): void
  getSymbols (cb: (error: Error, symbols: any) => void): void
  readState (cb: (error: Error, result: any) => void): void
  on<K extends keyof AdsClientEventDataMap> (data: AdsClientEventDataMap[K]): void
}

dionysiusmarquis avatar Aug 29 '19 13:08 dionysiusmarquis