Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update protobuf to v6 to resolve npm security warning #56

Merged
merged 3 commits into from
Sep 4, 2019
Merged

Conversation

feross
Copy link
Collaborator

@feross feross commented Aug 5, 2019

I need help confirming that this PR works as intended. I do not have a chromecast handy at the moment.

@dsteinman
Copy link

I tried it out on my code but looks like one of the parameters to protobuf is missing:

device: Device {
  id: '77b3f60a97fd61a1c7a1fed0067a4935',
  name: 'Family room TV',
  host: '192.168.1.226',
  port: 8009,
  type: 'Chromecast' }
Scanner - found device Family room TV (Chromecast) at 192.168.1.226:8009
MediaPlayer - New device Family room TV at 192.168.1.226:8009
buffer.js:508
    throw new ERR_INVALID_ARG_TYPE(
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "string" argument must be one of type string, Buffer, or ArrayBuffer. Received type undefined
    at Function.byteLength (buffer.js:508:11)
    at BufferWriter.write_string_buffer [as string] (chromecast-plugin/node_modules/castv2/node_modules/protobufjs/src/writer_buffer.js:68:22)
    at Type.CastMessage$encode [as encode] (eval at Codegen (chromecast-plugin/node_modules/castv2/node_modules/@protobufjs/codegen/index.js:50:33), <anonymous>:7:16)
    at Type.encode_setup [as encode] (chromecast-plugin/node_modules/castv2/node_modules/protobufjs/src/type.js:485:25)
    at Object.serialize (chromecast-plugin/node_modules/castv2/lib/proto.js:32:22)
    at Client.send (chromecast-plugin/node_modules/castv2/lib/client.js:127:25)
    at Channel.send (chromecast-plugin/node_modules/castv2/lib/channel.js:34:12)
    at fn.Controller.send (chromecast-plugin/node_modules/castv2-client/lib/controllers/controller.js:28:16)
    at fn.ConnectionController.connect (chromecast-plugin/node_modules/castv2-client/lib/controllers/connection.js:28:8)
    at Client.<anonymous> (chromecast-plugin/node_modules/castv2-client/lib/senders/platform.js:59:21)

@feross
Copy link
Collaborator Author

feross commented Aug 5, 2019

@dsteinman Thanks for giving it a shot. Can you run the code again, but enable the debug logs this time? Just run your script with the DEBUG environment variable set as follows:

DEBUG='castv2' node your-script.js

@dsteinman
Copy link

The only difference is these additional debug lines from right before that error.

  castv2 connecting to 192.168.1.226:8009 ... +0ms
  castv2 connected +480ms
  castv2 send message: protocolVersion=0 sourceId=sender-0 destinationId=receiver-0 namespace=urn:x-cast:com.google.cast.tp.connection data={"type":"CONNECT"} +1ms

@feross
Copy link
Collaborator Author

feross commented Aug 5, 2019

@dsteinman Thank you. That was very helpful. I just made some changes that should fix the problem. Can you try again once more with the latest code?

@dsteinman
Copy link

Looks like that might be working, but I can't 100% confirm because the rest of my code no longer works :/

PersistentClient - Family room TV - Connected client
PersistentPlayer - Family room TV - Try to join player
PersistentPlayer - Family room TV - Connected player
PersistentPlayer - Family room TV - Player joint

Looks okay and no errors. But castService.connect(device, callback) does not call the callback after what looks like a successful connection. And this now happens on the master branch too. So it might be something else, or maybe my Chromecast needs an update. I'll check back if I get it working.

@vincentcr
Copy link

Hi @feross I just tried this PR with my code (via https://github.com/thibauts/node-castv2-client), and it works just like before. So LGTM!

@vincentcr
Copy link

@feross please let me know if there are specific checks or tests you'd like me to do.

@feross
Copy link
Collaborator Author

feross commented Sep 4, 2019

@vincentcr This is good enough for me. I'll merge and release a new version.

@feross feross merged commit f8a41dd into master Sep 4, 2019
@feross feross deleted the update-deps branch September 4, 2019 23:26
@feross
Copy link
Collaborator Author

feross commented Sep 4, 2019

Released [email protected].

@vincentcr
Copy link

Awesome, thanks @feross

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants