feat!: add ws client to pull updated configuration #147
Conversation
kleyow
commented
Jun 3, 2022
•
kleyow
commented
- Ported over websocket client code from sdk-scheme-adapter
- Added feature to query websocket server for configuration on startup
- Added feature that will restart inbound/outbound servers on websocket configuration change message
- Conform config to camelcase/sdk-scheme-adapter since the client has merging config logic that depends on keys being the same
- Add parent server class that runs all servers/clients akin to sdk-scheme-adapter
kleyow
requested review from
vijayg10,
elnyry-sam-k,
gibaros,
mdebarros and
kirgene
There was a problem hiding this comment.
Hey @kleyow,
I don't see any major with the PR, just see my one comment about breaking changes with regard to the CASE change in the config, and also if any new additional configs required for this change would also be breaking?
I also recommend some updates to documentation...I.e. perhaps add something explaining (and/or linking) these changes for PM4ML support.
| "mgmtAPIWsUrl": "127.0.0.1", | ||
| "mgmtAPIWsPort": 4010 | ||
| }, | ||
| "inbound": { |
There was a problem hiding this comment.
Would UPPER-CASE to CAME-CASE be a breaking change here?
There was a problem hiding this comment.
Unsure. So...I'm going to make it one.
kleyow
changed the title
I'll ask someone for the relevant documentation if there is any and add it to the readme. |
| "release": "standard-version --releaseCommitMessageFormat 'chore(release): {{currentTag}} [skip ci]'", | ||
| "snapshot": "standard-version --no-verify --skip.changelog --prerelease snapshot --releaseCommitMessageFormat 'chore(snapshot): {{currentTag}}'", |
There was a problem hiding this comment.
standard-version marked as deprecated:
https://www.npmjs.com/package/standard-version
There was a problem hiding this comment.
Replaced with a fork that seems to have active contributions for now.
| switch (msg.msg) { | ||
| case MESSAGE.CONFIGURATION: | ||
| switch (msg.verb) { | ||
| case VERB.NOTIFY: |
There was a problem hiding this comment.
I think we need to pull the latest changes from mojaloop-connector:
infitx-org/mojaloop-connector@da5384d...master
| let serverInstance: Server | null | ||
|
|
||
| export async function start(server: Server): Promise<Server> { | ||
| await server.start() | ||
| const serverApp = server.settings.app as ServerApp | ||
| logger.info(`Service '${PACKAGE.name}' is running '${serverApp.api} API' @ ${server.info.uri}`) | ||
| serverInstance = server | ||
| return server | ||
| } | ||
|
|
||
| export async function stop(server: Server): Promise<Server> { | ||
| await server.stop() | ||
| serverInstance = null | ||
| return server | ||
| } | ||
|
|
||
| export async function restart(server: Server): Promise<void | Server> { |
There was a problem hiding this comment.
I think it would be cleaner to have a factory class rather than separate methods in a module.
So instead of
import someModule from './server';
someModule.start(); // <--- it's unclear which module we accesswe can have:
import { Server } from './server';
const server = Server::start(); // it's clear that we access Server classThere was a problem hiding this comment.
I'd rather just keep it functional for now vs extending off of hapi.server. I found it not that inituitive as well when messing about but lets leave that for a future refactor
