Refactor connection management

[mint-dewit]

Type of Contribution

This is a Code improvement

Current Behavior

Library consumers must keep track of how devices are created and initialised and are responsible for managing this. In addition, a device may not finish initialising until the first connection has been made.

Proposed Behavior

A Connection is an instance of an Integration Implementation (previously known as devices, this terminology proved to be a bit confusing as "TSR devices" connected to actual devices). TSR shall be responsible for creating and initialising the connections based upon a config provided by the library user. An integration must ensure to not get stuck during the initialisation.

Discussion

I think there may actually some use for devices not being initialised immediately as it prevents the timeline states from being sent into the integration when it isn't ready yet. At the same time, integrations should handle this "disconnected" state gracefully. Perhaps some additional life cycle hooks are required (i.e. to rediff the state from empty)

Status

This PR is part investigation and part implementation. Feedback is welcome at this time.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 61.58537% with 126 lines in your changes missing coverage. Please review.

Project coverage is 57.15%. Comparing base (29ad13d) to head (3d5ee7e).
Report is 11 commits behind head on release52.

Files with missing lines	Patch %	Lines
...ne-state-resolver/src/service/ConnectionManager.ts	76.65%	53 Missing ⚠️
packages/timeline-state-resolver/src/conductor.ts	40.62%	19 Missing ⚠️
...-state-resolver/src/integrations/casparCG/index.ts	0.00%	11 Missing ⚠️
...imeline-state-resolver/src/service/stateHandler.ts	0.00%	11 Missing ⚠️
...eline-state-resolver/src/integrations/osc/index.ts	11.11%	8 Missing ⚠️
...e-state-resolver/src/integrations/sisyfos/index.ts	0.00%	6 Missing ⚠️
...e-state-resolver/src/integrations/quantel/index.ts	37.50%	5 Missing ⚠️
...ne-state-resolver/src/integrations/vizMSE/index.ts	0.00%	5 Missing ⚠️
...line-state-resolver/src/integrations/vmix/index.ts	0.00%	2 Missing ⚠️
...eline-state-resolver/src/service/DeviceInstance.ts	0.00%	2 Missing ⚠️
... and 4 more

Additional details and impacted files

@@              Coverage Diff              @@
##           release52     #328      +/-   ##
=============================================
+ Coverage      56.74%   57.15%   +0.40%     
=============================================
  Files            130      131       +1     
  Lines          10104    10400     +296     
  Branches        2386     2553     +167     
=============================================
+ Hits            5734     5944     +210     
- Misses          4368     4454      +86     
  Partials           2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Julusian]

There are still a bunch of todos that need resolving, but I like the approach taken here and don't have any major problems with it.

I haven't read changes made to devices, but I suspect that some care will be needed in them. I know that some of them are doing assertions on when properties are defined, as they get defined by init, but now that init resolves immediately, will those assumptions still be correct?

[Julusian]

deviceOptions could be a parameter, which would avoid needing this guard which might want to throw.

This could actually be a race condition, if setConnections is called to remove a device between the start of createConnection and here, then the config could disappear.
So maybe this guard is a good thing, and should be documented as such? or it should continue as normal, with it following the normal destroy route soon after

[Julusian]

Is more cleanup needed?
This is discarding the thread, but the thread is probably still running at this point?

[Julusian]

should this be public, or should select methods be re-exposed on this conductor?

[mint-dewit]

This is pretty much ready and I'm leaving this in state I'm happy with but the unit tests appear to be very inconsistent for reasons that elude me so that will require some attention