feat: strict tsconfig and use threadedClass #60
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #60 +/- ##
===========================================
+ Coverage 87% 87.01% +<.01%
===========================================
Files 112 113 +1
Lines 2701 2733 +32
Branches 380 409 +29
===========================================
+ Hits 2350 2378 +28
- Misses 345 348 +3
- Partials 6 7 +1
Continue to review full report at Codecov.
|
Julusian
force-pushed
the
feat/strict-tsconfig
branch
from
c9bac04
to
3b79a9e
Compare
… mask). Returns true if anything was changed
…kFlags (when appropriate)
…ons when doing multiple uploads
…ed for just before the wrap
…ting the connection
…ternative when commands are constructed manually
I think we should follow the idea that we should only error for a case that we did not expect to find. So in the case for a device not having a DVE that should be handled / ignored, possibly applyToState shouldn't even throw. In any other case it's unexpected and should be logged as an error.
We could do that, but at the same time it's not a requirement for any of the current users. We could also wait until someone needs to do that. I imagine it can be done in a non-breaking way |
yeah, so should that be a warn/info/log/debug/trace level message or not at all logged?
I would like to throw in the unit-tests as a 'user' of this requirement. |
It makes sense to add debug logging for it, I imagine a scenario where someone wants to make sure the handling of those failures is or is not the root cause of some bug they are investigating and in that case it would be nice to have that logged. If there are current usecases for the serializing/deserializing of state object I guess it's a good idea to make that possible. I think we could either define the interfaces, have classes with initializers and move out the helper methods to be utility functions or define everything as interfaces and have a utility function for generating a state with initialized values. |
…y logged at the debug level. Tidy up the logging to be events emitted from the Atem class.
|
@baltedewit sorry for the large touching every command changes happening again. This now has the state purely as interfaces, with AtemStateUtil containing all the Then I have made a custom Error type for the expected invalid id errors thrown in applyToState. And changed how logs are passed out of the library. It now has 3 levels of emit for logs ('error', 'info' and 'debug') |
|
Change pushed to atem-state to match this state refactor. |
|
Both changes look good to me! |
Some non-blocking tasks moved to #73
This enables some stricter typescript rules. Primarily to enable
strictPropertyInitializationto ensure that all class properties are initialised before they can be used.Additionally, fork has been replaced with threadedClass which is more durable and allows for using workers instead of fork when using recent enough node.
The connection handling logic has been thoroughly reviewed and many bugs have been fixed. There were some problems with retransmits when the packet id would wrap, which was causing instability with media upload, and other possible connection deadlocks if the connection is unstable and started to hit some timeouts.
Data parsing has been loosened, as we cannot guarantee data ranges, and that they wont change. We now parse the command, and then decide whether to apply it to the state based on whether the ids are within known good ranges for the current device. We assume that any non-id value received is valid, and we assume all values in outbound commands are within range (including ids). This should fix #24 #15 #61 and prevent similar issues cropping up in future.
Typescript is a bit loose in its checks, so some fields can be undefined even though their typings do not indicate that.
Using fork to open the child process has caused some stability problems, with it getting orphaned under certain circumstances. #30 #40 #68 #65 (possibly more)
This is a massive overhaul of a majority of the library.
Every command class had to be refactored to allow the typings to work. AbstractCommand has been replaced with a couple of separate base classes & interfaces. Different for each of serializable and deserializable commands.
Other benefits of this is that various properties are now readonly when possible, and there is less dependencies on ensuring 'random' properties are set at the right points.
Threadedclass does come with 4-5% more cpu usage (19% vs 14%) than the fork implementation while doing still media uploads. But with that we get a safer wrapper around the child process, both in typings and in durability. The performance drop hasnt been investigated fully yet, but that could be done if this is found to be a problem.
Additionally, media is now a lot quicker to upload than previously.
in node 8 with fork I was getting ~1400ms per frame, and that is down to ~1000ms when running this in node 12. node8 still performs around ~1400ms, the benefit here comes only when running newer node.