Skip to content

Add DeviceData and eventHandlers.js#14

Open
liberty-rowland wants to merge 3 commits intonimiq:masterfrom
liberty-rowland:DeviceData
Open

Add DeviceData and eventHandlers.js#14
liberty-rowland wants to merge 3 commits intonimiq:masterfrom
liberty-rowland:DeviceData

Conversation

@liberty-rowland
Copy link

@mar-v-in Based on our conversation around nimiq-network/developer-reference#24

This PR does a few things:

  1. Adds knowledge of deviceData to PoolAgent
  2. Adds public read-only properties to PoolAgent, since we will now be exposing it to pool owners. Properties are: deviceId, deviceLabel, mode, isRegistered
  3. Added eventHandlers.sample.js and support for eventHandlers.js which allows pool owners to easily add handlers for events. I started with onRegister and beforeRegister but this can be expanded on.

@liberty-rowland liberty-rowland mentioned this pull request May 16, 2018
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description is misleading. The PoolAgent was already created at this time (because it is created before the first register arrives). There is also no reason why the PoolAgent instance is not a param on this function.

My suggestions would be
a) add PoolAgent param to beforeRegister
b) rename beforeRegister to onRegisterMessage
c) fix the description to state that it is fired before the message is processed by the PoolAgent
d) also rename onRegister to onRegistrationCompleted to make clear that it is fired after the registration.

src/PoolAgent.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use ES6 getters in our codebase instead.

@liberty-rowland
Copy link
Author

@mar-v-in Addressed your concerns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this one be async as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I'd leave it synchronous since I was catching with try/catch but I suppose this will still work as async.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, exceptions do work across async/await

src/PoolAgent.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing await?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

@liberty-rowland
Copy link
Author

Thanks @mar-v-in, addressed second round of comments.

* Fired when a REGISTER message is received, before being processed by the
* PoolAgent. Good time to perform validation or mutation of message data.
* @param {PoolAgent} agent - The Agent for the newly registered device.
* @param {Object} msg - The full register message. This is not a copy; any

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msg should definitely be a copy

@Zolmeister
Copy link

This feels like it should be middleware.js (instead of having events trigger side-effects)

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

Successfully merging this pull request may close these issues.

3 participants

Comments