agsh / onvif

ONVIF node.js implementation
http://agsh.github.io/onvif/
MIT License
693 stars 236 forks source link

Fix getVideoSources response #146

Closed bl0ggy closed 3 years ago

bl0ggy commented 4 years ago

The response should be a list of video sources. This breaks backward compatibility!

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.1%) to 82.126% when pulling 478971a67fd788d7a9921b73c603dc778e3e055c on bl0ggy:fix_getVideoSources_response into 1a995579d7917ddd555304804eeb94af38408fc9 on agsh:master.

RogerHardiman commented 4 years ago

in media.js around line 39 it says

           * Video sources
           * @name Cam#videoSources
           * @type {Cam~VideoSource|Array.<Cam~VideoSource>}
           */

So the definition is a single item or an array of items. Your change means it is always an array. I'll refer to @agsh for this one

But if we do the change then we also have to change the comments to

           * Video sources
           * @name Cam#videoSources
           * @type {Array.<Cam~VideoSource>}
           */

and also around line 22

/**

  • @callback Cam~GetVideoSourcesCallback
  • @property {?Error} error
  • @property {|Array.<Cam~VideoSource>} videoSources
  • @property {string} xml Raw SOAP response */
bl0ggy commented 4 years ago

Oh yes, sorry I forgot to check about JSDoc. What makes me switch to full Array is that many times I got unexpected errors while browsing objects supposed to be lists (from ONVIF wsdl specs), but were in reality simple objects because there is only one element inside. PR #147 is similar, and there is another one I plan to do for resolutionsAvailable and xxxxProfilesSupported from the response of GetVideoEncoderConfigurationOptions.

But as it breaks backward compatibility I understand if this PR is not accepted. We should however change the JSDoc of #147 to return either an object or an Array.

RogerHardiman commented 4 years ago

What makes me switch to full Array is that many times I got unexpected errors while browsing objects supposed to be lists (from ONVIF wsdl specs), but were in reality simple objects because there is only one element inside.

All the original software was written by @agsh so we need to check with him. But my own preference would be to always return an Array, even with just 1 element.

The JSDocs say some things can be an Item or an Array. But they do not say an Array must be 2 items or more. So in my view, an array of 1 item is fine as it still meets the JSDocs.

Let's see what Andrew says but we can always have a new Major Version number.