fix!: proper support for Profile 1 <mosReqAll>, <mosObj> and <mosListAll> (SOFIE-2890)

[nytamin]

About the Contributor

This pull request is posted by me on behalf of SuperFly.tv.

Type of Contribution

This is a:

BREAKING Feature

Current Behavior

Disclaimer: To be honest, I think that the mos protocol spec is a tad unclear on how this REALLY should work, but I believe that this is how it should be. Ref: Mos Protocol; mosReqAll.

Currently, when mosDevice.sendRequestAllMOSObjects() is called, a <mosReqAll> message is sent, and a <mosListAll> is expected as a reply.

This is not according to the spec, since there should be a mosAck, followed (asynchronously on the other server initiated lower socket, I believe) by mosListAll - if pause = 0, or mosObj - if pause > 0.

New Behavior

When mosDevice.sendRequestAllMOSObjects() is called, a <mosReqAll> message is sent, and a <mosAck> is expected as a reply.

Subsequent <mosListAll> or <mosObj> messages are parsed and triggers the callback set up using mosDevice.onMOSObjects(cb: (objs: IMOSObject[]) => void).

Since mosDevice.onMOSObjects() is now required to be set up for Profile 1 in strict mode, this PR is technically a breaking one.

Testing Instructions

• If possible, testing with a MOS device to ensure that the exchange logic works.

(I have implemented this using the mos protocol only, not tested with other MOS devices.)

Other Information

Status

• [x] PR is ready to be reviewed.
• [ ] The functionality has been tested by the author.
• [x] Relevant unit tests has been added / updated.
• [x] Relevant documentation (code comments, system documentation) has been added / updated.

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (0368fe8) 77.08% compared to head (34fba90) 77.26%. Report is 1 commits behind head on master.

Files	Patch %	Lines
packages/connector/src/MosDevice.ts	90.00%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #89 +/- ## ========================================== + Coverage 77.08% 77.26% +0.17% ========================================== Files 65 65 Lines 3522 3532 +10 Branches 799 800 +1 ========================================== + Hits 2715 2729 +14 + Misses 724 720 -4 Partials 83 83 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jstarpl]

Hello!

Thank you for contributing to the Sofie Project!

If you haven’t already, please give our contribution guidelines a read. We will reach out to you to schedule an initial discussion regarding this (more details to follow).

[jstarpl]

Because we have no way to test this, we will reach out to some users of mos-connection, to see if they are able to verify that this works. If they can't, because they don't use Profile 1, we will accept this and as-is and look forward to any Bug issues if there are problems with this implementation.

I expect to have a go-no go decision before the end of the week.

[jstarpl]

We have not been able to find anyone who may have insight into this feature so Sofie Team has decided to jest merge this as-is, and hope for Bug Reports if someone using this library with Profile 1 has any feedback.

[Julusian]

Question, how breaking is this 'breaking change'? When merged this will trigger a 4.0 release which seems a bit unnecessary as it sounds like this functionality is broken anyway so wont be used by anyone. Or am I misinterpreting?

[jstarpl]

Question, how breaking is this 'breaking change'? When merged this will trigger a 4.0 release which seems a bit unnecessary as it sounds like this functionality is broken anyway so wont be used by anyone. Or am I misinterpreting?

Well, we had the same discussion this morning and since we're already on 3.x, it didn't seem like that big of a deal to do a major release. The signature of the method changes drastically, as well as how it needs to be used (data received as a result of Promise vs async messages callbacks). I guess we could say that this never worked, so it shouldn't break any actual code, but then - what's the downside of a major release?

[jstarpl]

@nytamin It seems that the unit tests are failing here - could you check that? Otherwise, this is good to be merged.