Skip to content

fix: fix streambundle error after port to TypeScript #1945

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

Merged
merged 2 commits into from
Apr 24, 2025

Conversation

bkeepers
Copy link
Contributor

@bkeepers bkeepers commented Apr 16, 2025

This fixes an error reported by @KEGustafsson introduced in #1918. I have not been able to recreate it, but apparently there are instances where update.values is present, but is not an array. Looking back at #1918, it looks like the old code did have an if conditional, so there must be instances where it is null/undefined. I updated the types to be consistent with the implementation as well.

While I was looking at that, I noticed that app is injected into stream bundle, but is unused, so I removed it in another commit.

@bkeepers bkeepers force-pushed the fix-ts-streambundle branch from a6a19af to 6b5806c Compare April 16, 2025 13:24
@tkurki
Copy link
Member

tkurki commented Apr 16, 2025

I can’t think of a use case for values not being an array, so i would rather implement a check-and-discard in handleMessage so that downstream one can expect valid data. @panaaj @sbender9 @KEGustafsson ?

@tkurki tkurki added the chore label Apr 16, 2025
@sbender9
Copy link
Member

Just don't forget about meta deltas, where "values" may be undefined.

@bkeepers bkeepers force-pushed the fix-ts-streambundle branch from 6b5806c to ec7d088 Compare April 16, 2025 16:04
@KEGustafsson
Copy link
Contributor

This and #1942 seemed to fix recently seen problems.
I could try to spend couple of minutes to check what actually caused issues.

@KEGustafsson
Copy link
Contributor

KEGustafsson commented Apr 16, 2025

It is meta causing this issue . Added try-catch to collect deltas and here is small piece of the log throwing error.

Apr 16 17:23:34 { "meta": [ { "path": "notifications.mob", "value": { "supportsPut": true } } ], "$source": "freeboard-sk", "timestamp": "2025-04-16T17:23:34.205Z" }
Apr 16 17:23:34 { "meta": [ { "path": "notifications.fire", "value": { "supportsPut": true } } ], "$source": "freeboard-sk", "timestamp": "2025-04-16T17:23:34.209Z" }
Apr 16 17:23:34 { "meta": [ { "path": "notifications.sinking", "value": { "supportsPut": true } } ], "$source": "freeboard-sk", "timestamp": "2025-04-16T17:23:34.212Z" }
Apr 16 17:23:34 { "meta": [ { "path": "notifications.flooding", "value": { "supportsPut": true } } ], "$source": "freeboard-sk", "timestamp": "2025-04-16T17:23:34.214Z" }
Apr 16 17:23:34 { "meta": [ { "path": "notifications.collision", "value": { "supportsPut": true } } ], "$source": "freeboard-sk", "timestamp": "2025-04-16T17:23:34.216Z" }
Apr 16 17:23:34 { "meta": [ { "path": "notifications.grounding", "value": { "supportsPut": true } } ], "$source": "freeboard-sk", "timestamp": "2025-04-16T17:23:34.218Z" }
Apr 16 17:23:34 { "meta": [ { "path": "notifications.listing", "value": { "supportsPut": true } } ], "$source": "freeboard-sk", "timestamp": "2025-04-16T17:23:34.219Z" }

@bkeepers bkeepers force-pushed the fix-ts-streambundle branch 2 times, most recently from 28825dd to 4833dcb Compare April 16, 2025 21:27
@bkeepers bkeepers force-pushed the fix-ts-streambundle branch from 4833dcb to 4e4fcd0 Compare April 16, 2025 21:37
Copy link
Contributor Author

@bkeepers bkeepers left a comment

Choose a reason for hiding this comment

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

I can’t think of a use case for values not being an array, so i would rather implement a check-and-discard in handleMessage so that downstream one can expect valid data.

Sounds good, I updated handleMessage. I tried adding some tests for this behavior, but adding unit tests was going to require some refactoring that belongs in a pull request of its own.


// No valid updates, discarding
if (data.updates.length < 1) return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prettier's 80 char line limit is so annoying. Most of these changes are because line 282 is now just over 80 characters, and now it's hard to see what else I actually changed. Anyone opposed to changing it to 100 characters?

Here's a link to view the diff without whitespace changes

@tkurki tkurki merged commit c5f9059 into SignalK:master Apr 24, 2025
3 checks passed
@bkeepers bkeepers deleted the fix-ts-streambundle branch April 24, 2025 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants