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

issue #29 . Sending as Binary. #31

Closed
wants to merge 2 commits into from

Conversation

dmill-bz
Copy link
Collaborator

This should correct the issue with the client not communicating the mimeType to the server.

What the PR covers:

  • WebSocketGremlinConnection now sends messages as binary and masked
  • GremlinClient now packs the request messages in binary with the included mimeType headers
  • GremlinClient now unpacks the binary Blob response sent back from the server

I wasn't able to get npm run test:node to work, got some strange legacy node error ; I think my env isn't setup correctly. So this will need testing as I only have some manual testing going for this PR at the moment.

FYI, my manual testing covers:

  • application/json simple send and reading response (haven't tried any streaming)
  • Custom mimeType application/gremlinbin same as above

@dmill-bz
Copy link
Collaborator Author

Changed the styling a bit. Should be a better match for babel.

@jbmusso
Copy link
Owner

jbmusso commented Mar 17, 2016

Thanks for sending this Dylan. Unfortunately, FileReader is a browser API and doesn't work in a Node.js environment. I pushed a branch (see 852574f?diff=split) where I converted your code to use Buffer.toString() (Buffer is a core Node.js module). I believe Buffer is shimmed when using browserify (I think via https://github.com/feross/buffer). Hopefully this should work in all environments (browsers/Node.js).

Can you checkout this and see if that works in a browser when using browserify?

@dmill-bz
Copy link
Collaborator Author

Ah good I didn't realize that!. I'll close this and test it out.

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.

None yet

2 participants