Skip to content

feature: new immediate-style API to replace Promise/events …#130

Merged
tkurki merged 5 commits intoSignalK:masterfrom
sarfata:refactor/immediate-api
Aug 23, 2018
Merged

feature: new immediate-style API to replace Promise/events …#130
tkurki merged 5 commits intoSignalK:masterfrom
sarfata:refactor/immediate-api

Conversation

@sarfata
Copy link
Contributor

@sarfata sarfata commented Aug 16, 2018

Collection of changes that lead to:

  • a new API that should be easier to use
  • compatibility with webpack

Biggest change is the last one:

Replace the mix of promises and events in the old API by a simpler API
that returns a SignalK Delta object, null if there was no valid data
(invalid gps fix, unsupported sentence or partial AIS message for
example) and throws exception when something goes wrong (invalid
checksum, invalid data fields, etc).

Not using promises makes it easier for callers to use the library and
should result in less memory usage and slightly improve performance.

We need to merge SignalK/specification#498 and update the dependency before we are really compatible with webpack.

Loading the list of hooks from the filesystem at runtime was very fancy
but it does not work well when the project is webpacked and used in a
browser.
lodash was used in only one place in the project, as was lib/types.js. I
have inlined them like we do in most other places.
This changes the hook API. Instead of receiving a reference to the
parser (which only VDM used for AIS sessions) and the sentence parts as
the second argument, the hooks now receive the sentence parts first and
the session as the second argument.

Because most hooks do not use it, they can just ignore the second
argument making them completely stateless.
Some sentences are supported but not listed in the README. Add them.
Replace the mix of promises and events in the old API by a simpler API
that returns a SignalK Delta object, null if there was no valid data
(invalid gps fix, unsupported sentence or partial AIS message for
example) and throws exception when something goes wrong (invalid
checksum, invalid data fields, etc).

Not using promises makes it easier for callers to use the library and
should result in less memory usage and slightly improve performance.
@sarfata sarfata force-pushed the refactor/immediate-api branch from d73e657 to 4c00fed Compare August 16, 2018 19:45

parse (sentence) {
if (typeof sentence === 'string' && sentence.trim().length > 0) {
this.emit('nmea0183', sentence.trim())
Copy link
Member

Choose a reason for hiding this comment

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

This is something we want to keep: emitting the raw sentences. These are available within the server for plugins and for example over tcp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be hard to emit the sentences from the same place where you call parser.parse()?

I can re-introduce it but it seems pretty orthogonal to the library itself. There is absolutely no value added besides .trim().

@tkurki
Copy link
Member

tkurki commented Aug 17, 2018

I have my SK server glasses on, but there the logical component that knows what is supposedly an NMEA0183 sentence is the parser. We aren't quite doing it though, because every line in the input is emitted as NMEA0183, no matter what.

On second thought we can move this functionality to the NMEA0183 provider. Just something to do when we deploy this in the server. These changes bump the major version anyway.

@tkurki tkurki merged commit 59d4934 into SignalK:master Aug 23, 2018
@tkurki
Copy link
Member

tkurki commented Aug 23, 2018

Are we done here? Could you take care of changes in the server next?

@sarfata
Copy link
Contributor Author

sarfata commented Sep 1, 2018

@tkurki I am looking into the server now. Can you publish version 2.5.0 of the package to npm?

@tkurki
Copy link
Member

tkurki commented Sep 1, 2018

Master is no longer backwards compatible, publishing it as 2.x would break things. Let’s work out the kinks off git and then publish 3.0.

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.

2 participants