patrickmichalina / onvif-rx

📹 Communicate with ONVIF devices and cameras in server and browser environments.
MIT License
22 stars 11 forks source link

Incorrect Parameter Destructuring #50

Closed Mudrekh closed 2 years ago

Mudrekh commented 2 years ago

Hi @patrickmichalina

An error was introduced with this commit. The parameters are incorrectly destructured, especially when they are strings. Here is an example highlighting the issue.

var __assign = function () {
  __assign = Object.assign || function __assign(t) {
    for (var s, i = 1, n = arguments.length; i < n; i++) {
      s = arguments[i];
      for (const p in s) if (Object.prototype.hasOwnProperty.call(s, p)) t[p] = s[p];
    }
    return t;
  };
  return __assign.apply(this, arguments);
};

const params = {
  trt_Name: 'recording',
  trt_Token: 'recording'
};

Object.keys(params).forEach(key => params[key] === undefined ? delete params[key] : {});
const result2 = Object
  .keys(params)
  .reduce((acc, curr) => params[curr] === undefined ? acc : __assign(__assign({}, acc), params[curr]), {});
const result3 = Object
  .keys(params)
  .reduce((acc, curr) => params[curr] === undefined ? acc : { ...acc, ...params[curr] }, {})

console.log(params);
console.log(result2);
console.log(result3);

This is the result:

{ trt_Name: 'recording', trt_Token: 'recording' }
{ '0': 'r',
  '1': 'e',
  '2': 'c',
  '3': 'o',
  '4': 'r',
  '5': 'd',
  '6': 'i',
  '7': 'n',
  '8': 'g' }
{ '0': 'r',
  '1': 'e',
  '2': 'c',
  '3': 'o',
  '4': 'r',
  '5': 'd',
  '6': 'i',
  '7': 'n',
  '8': 'g' }

The first result is the 'expected' output when passed the params with the original 'clean' function. The second version is the TS compiled version, and the 3rd is the current TS.

patrickmichalina commented 2 years ago

Ok I'll write a fix. I see the example inputs and can make a test to validate it. I never use "delete" keyword and that's why I removed it from your PR.

patrickmichalina commented 2 years ago

Think it is easy

{ ...acc, [curr]: params[curr] }

Or

Object.assign(acc, { [curr]: params[curr] })

Mudrekh commented 2 years ago

Ok I'll write a fix. I see the example inputs and can make a test to validate it. I never use "delete" keyword and that's why I removed it from your PR.

Yea, I know there are performance hits w/delete but AFAIK you really only run into that ineffciency when running deletes on large/complex objects.

I believe either option works, with the second being more 'backwards' compatible.

Mudrekh commented 2 years ago

@patrickmichalina Do you know when you could publish these changes? I can submit a PR if that would help. I would love to get a npm version in a project I'm actively working on!

patrickmichalina commented 2 years ago

@Mudrekh as a matter of principle I just never use delete, cleaner to avoid mutations in general. I will release the change now.