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

Promise support #14

Open
1 task done
roccomuso opened this issue Aug 8, 2018 · 12 comments
Open
1 task done

Promise support #14

roccomuso opened this issue Aug 8, 2018 · 12 comments
Assignees
Milestone

Comments

@roccomuso
Copy link
Owner

roccomuso commented Aug 8, 2018

  • Support for Promises.
@roccomuso roccomuso added this to the v2.0.0 milestone Aug 8, 2018
@ovi1337
Copy link
Collaborator

ovi1337 commented Aug 8, 2018

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

@ovi1337
Copy link
Collaborator

ovi1337 commented Aug 8, 2018

Your question was regarding promises right?

@roccomuso
Copy link
Owner Author

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.

@ovi1337
Copy link
Collaborator

ovi1337 commented Aug 8, 2018

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.

@roccomuso
Copy link
Owner Author

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?

@ovi1337
Copy link
Collaborator

ovi1337 commented Aug 8, 2018

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.

@roccomuso
Copy link
Owner Author

roccomuso commented Aug 8, 2018

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.

@ekuzu
Copy link

ekuzu commented Aug 28, 2018

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.

@dionysiusmarquis
Copy link

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

@roccomuso
Copy link
Owner Author

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

@ovi1337
Copy link
Collaborator

ovi1337 commented Aug 29, 2019

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.

@dionysiusmarquis
Copy link

dionysiusmarquis commented Aug 29, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants