ReferenceError: val is not defined

[dcposch]

Currently, node-castv2 depends on an old version of protobufjs (3.x instead of the latest 5.x), which depends on an old version of bytebuffer, which has the following error:

.../bytebuffer/src/types/strings/vstring.js:67:63: 'val' is not defined.

I saw ReferenceError: val is not defined in our telemetry for WebTorrent Desktop.

That's weird, I thought-- reference errors are almost like syntax errors, they can be caught with a linter and should never happen at runtime.

So I ran standard on our whole node_modules folder, which crunched for a few minutes and then found the error above.

I think the easiest way to fix it is for node-castv2 to upgrade to the latest protobufjs.

[dcposch]

I saw you guys actually tried to upgrade protobufjs recently, but it didn't work out:
#31
#34

What do we have to fix?

@thibauts @amilajack

[thibauts]

I can't answer for @amilajack, so the best way to learn here is probably to try it out. It's probably pretty straightforward to do.

[amilajack]

Sorry for the late response. I might have not seen the notification.

I reverted all the changes I made:

https://github.com/thibauts/node-castv2/pull/31/files
https://github.com/thibauts/node-castv2/pull/34/files

So nothing should be broken (at least by my changes), since the update practically never occurred.
@thibauts again, sorry for the horrible PR.

@dcposch updating protobuf is risky because testing it is very difficult. You need every kind of chromecast device (to be safe)

[thibauts]

It would still be nice to upgrade protobufjs. So if someone wants to give a try, I'll merge the changes.

[amilajack]

@thibauts, but we need a formal way of testing. Without that, I think updating deps is risky. Maybe we can write a script that runs node-cast against a local network and the tester will manually have to check if the content is streamed to the device. (Doesn't seem like a very elegant testing method but it guess it kind of works). The script should be run by multiple testers on multiple devices to increase the guarantee that it will work as expected.

[thibauts]

Unless I'm mistaken, the newest version of protobufjs deals with the same version of protobuf so nothing should break. Also, protocol buffer is designed from the beginning to support seamless version upgrades.

Testing on one device should do the trick. If the module passes authentication and is able to get a status update, every possible packet formats will have been read / written on the wire...

[amilajack]

The API of the newest protobufjs version is not backwards compatible. We need to update the code of this module to reflect the new API changes.

. If the module passes authentication and is able to get a status update

Is it possible to do this kind of testing on a VM so we can run tests on Travis CI or appveyor?

[thibauts]

I just looked up the API, it seems pretty much backwards compatible. Can you show me what breaks exactly when run ?

[thibauts]

Regarding testing, you could build a bare bones server with the included Server class to test against. Look here https://github.com/thibauts/node-castv2/blob/master/lib/server.js

If the issue is related to a protocol upgrade on some Chromecasts you'll miss it though. So Travis CI may not be the answer here.

[thibauts]

Closing for inactivity

[superhawk610]

A moderate vulnerability has been discovered in versions of protobuf before 5.0.3. Details can be found here.

From reading past discussion on this, it seems that the older version was kept due to lack of ability to test across all Chromecast devices, correct? I would be willing to work on evaluating how viable it is to update the dependency if you wouldn't mind enumerating the test that would be necessary to proceed.

[superhawk610]

Alright, I'm back a few months later armed with a better understanding of protobufs. I've upgraded the version and accounted for breaking changes in protobuf@^6.8.8, and it seems that everything is working as expected. If anyone else is still tracking this issue, let me know and I'll open a PR against this repo.

EDIT: Pull request is here if anyone would like to review.