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

[mds-web-socket] extensible entities #310

Merged
merged 18 commits into from
May 19, 2020
Merged

[mds-web-socket] extensible entities #310

merged 18 commits into from
May 19, 2020

Conversation

twelch
Copy link

@twelch twelch commented May 9, 2020

This is an initial stab at making the web socket package not so tightly coupled to base MDS VehicleEvent and Telemetry types. It allows any type of JSON to be sent. The Nats EventProcessor sending events on the other end seems to use open-ended generic typing already and the WS server code was simply casting to more specific types. Not clear to me if that gains anything over accepting Json type.

Feedback welcome on approach, and whether other parts that would need this treatment. The WS client seems unused and possibly not in need of migration.

PR Checklist

  • simple searchable title - [mds-db] Add PG env var, [config] Fix eslint config
  • briefly describe the changes in this PR
  • mark as draft if should not be merged
  • write tests for all new functionality

@twelch twelch requested review from avatarneil and mdurling May 9, 2020 15:46
@twelch
Copy link
Author

twelch commented May 9, 2020

A next step on this PR might be allowing the accepted entities to be extended or overridden somehow. Or just removing the entity checking altogether if we assume all senders are to be trusted to send the right thing (thinking out loud)

@twelch
Copy link
Author

twelch commented May 10, 2020

I made a pass at the ability to override the supported events/entities withing WebSocketServer. Admittedly it's without clarity on where this needs to go, I'm playing around a bit.

Essentially an event-to-entity mapping gets passed in as a function param and it is used to check incoming events and only pass on those that are present in the config. The default mapping is the equivalent to what was previously hard-coded (events, telemetries only).

So if one wanted to override the default mapping with new events/entities, based on how WebSocketServer() is invoked in containers-images/mds-web-sockets/index.ts it would mean the new config would get passed in here, which I'm not sure feels right. A different approach might be to turn WebSocketServer into a class and it can be extended with different configuration. Feedback welcome @avatarneil

WebSocketServer({ event: 'EVENTS', telemetry: 'TELEMETRIES', newEvent: 'NEWENTITY' })

Type signature is
export const WebSocketServer = (eventEntityMap: EventEntityMap = defaultEventEntityMap) => {

With the web-socket types having been expanded to

export const ENTITY_TYPES = ['EVENTS', 'TELEMETRIES'] as const

export type ENTITY_TYPE = typeof ENTITY_TYPES[number]

export type WS_EVENT_TOPIC = 'event' | 'telemetry'

export type EventEntityMap = { [S in WS_EVENT_TOPIC]: ENTITY_TYPE }

Copy link

@avatarneil avatarneil left a comment

Choose a reason for hiding this comment

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

@twelch Couple of thoughts:
We could use a generic string literal array instead of a map for the types, e.g.

const ENTITY_TYPES = ['event', 'telemetry'] as const
const ENTITY_TYPE = typeof ENTITY_TYPES[number]

This would allow us to pass in a generic at init time for the server, something like

const WebSocketServer = <T extends readonly string[]>(entityTypes: T = EntityTypes) => ...

And then we could define a "temporary" type within the server that indexes that (so we can actually pull out the entries) the same way we do with the defaults. This would allow the configuration to be automagically inferred if the client specifies the entityTypes.

The one thing that's lost with the approach I outlined above, is that we lose the all-caps that is desired for our payload protocol, but to get around that we could just use .toUpperCase() on the entityTypes.

The main benefit IMO is that we can drop all of the loose string typings, and actually have the types be derived from the entityTypes string literal array.

Copy link

@mdurling mdurling left a comment

Choose a reason for hiding this comment

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

Looks fine aside from @avatarneil suggestions.

@twelch
Copy link
Author

twelch commented May 11, 2020

@avatarneil toUppercase isn't quite enough, note 'TELEMETRIES' is not the uppercase of 'telemetry'. Are you proposing I should introduce 'TELEMETRY' and 'EVENT' as a breaking change to WS protocol replacing 'TELEMETRIES' and 'EVENTS'?

Why is uppercase "desired"? Just noting we could avoid the transform altogether potentially.

@avatarneil
Copy link

avatarneil commented May 11, 2020

@twelch Ahh, right. Yeah, I think that consolidating down TELEMETRIES into TELEMETRY (or telemetry) worth doing -- docs and the FE will need to be updated. Lowercase is probably fine, having it be uppercase was a totally arbitrary decision I made IIRC

@twelch
Copy link
Author

twelch commented May 12, 2020

I'm now focused on generalizing the Clients class, which stores subscriptions using hardcoded entity names

@twelch
Copy link
Author

twelch commented May 13, 2020

@avatarneil this is ready for another review, thanks.

@twelch twelch requested a review from levi217 May 13, 2020 03:48
@twelch twelch self-assigned this May 13, 2020
@twelch twelch marked this pull request as ready for review May 14, 2020 17:02
Copy link

@avatarneil avatarneil left a comment

Choose a reason for hiding this comment

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

LGTM baring the one rename comment @drtyh2o made

packages/mds-web-sockets/server.ts Outdated Show resolved Hide resolved
packages/mds-web-sockets/server.ts Outdated Show resolved Hide resolved
packages/mds-web-sockets/types.ts Outdated Show resolved Hide resolved
@twelch twelch changed the title [mds-web-socket] Switch to more generic Json typing [mds-web-socket] extensible entities May 19, 2020
@twelch
Copy link
Author

twelch commented May 19, 2020

Merged latest goodness and made requested changes. Please see last comment @avatarneil and advise on merge timing. Thanks.

@twelch twelch merged commit f7a7361 into develop May 19, 2020
@marie-x marie-x deleted the feature/ws-generic branch March 10, 2021 16:57
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