fix(core): fix insufficient comparison in applyStateAndEmitEvents

[maxileith]

About this Pull Request

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  • fix, since the introduction of outputDevices to this library, it is required to compare state changes of the attributes with a deep comparison instead of a flat one, since [{"test": 1}] == [{"test": 1}] is false in JavaScript. Therefore a deep comparison is required in order to fix #295
• What is the current behavior? (You can also link to an open issue here)
  • …
• What is the new behavior (if this is a feature change)?
  • …
• Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
  • …
• Other information:
  • …

Pull Request Checklist

• [x] My code follows the code style of this project
  • Run npm run lint to double check
• [ ] My change requires a change to the documentation
  • [ ] I have updated the documentation accordingly
• [ ] I have added tests to cover my changes
  • Run npm run test to run the unit tests and npm run coverage to generate a coverage report
• [x] All new and existing tests passed
• [x] My commit messages follow the commit guidelines

[sebbo2002]

I also thought about it, but I assumed that atvscript only generates the event if there is a change. This is not necessarily the case with the default state with many fields, as it contains a large number of fields, thus the comparison. Therefore, when considering "additional dependency for deep-compare" / "hacky compare with toJSON" / "no compare", I opted for the latter, as the functionality is retained in the end. Do I see this wrong?

[maxileith]

I am not 100% sure wether or not the atvscript reports a change of output_devices only if there is a true change. Tbh, I would not rely on that ...

"hacky compare with toJSON"

I wouldn't consider this an option, since there is always the possibility of the object attributes beeing out of order and then there would be false positives as well.

[maxileith]

Would you consider it beeing it a vaiable option if I write a custom deepEqual? This way you would not have an additional dependency.

Edit: I just started doing that. Turns out not to be trivial ... haha

[sebbo2002]

Would you consider it beeing it a vaiable option if I write a custom deepEqual? This way you would not have an additional dependency.

Edit: I just started doing that. Turns out not to be trivial ... haha

Haha. But is that even necessary? As I said, I assume that the events are only thrown by atvscript if there is a given change.

Edit: Just saw your first comment. To be honest, I would rely on that as long as the opposite is shown. That can then be implemented without requiring a Breaking Change...

[maxileith]

@sebbo2002 up to you. It is probably sufficient. However, I somehow find it very unpleasing to compare arrays flat. I have implemented the deepEqual method. Up-to-you wether you wanna merge it or not. It was a fun problem to solve anyhow.

[sebbo2002]

@sebbo2002 up to you. It is probably sufficient. However, I somehow find it very unpleasing to compare arrays flat. I have implemented the deepEqual method. Up-to-you wether you wanna merge it or not. It was a fun problem to solve anyhow.

Yes, you're right. It's very dirty, even if it works. All right, let's agree on a compromise. Because to be honest, a full-fledged deepEqual implementation seems a bit too much of a good thing to me. What do you think of this solution? https://github.com/sebbo2002/node-pyatv/pull/300

[maxileith]

@sebbo2002 I think this is a good middle ground 👍

[sebbo2002]

Nice. I close this PR then. I merged the other one then. Thank you.