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

Implement deno_handle_msg_from_js in Rust #514

Merged
merged 3 commits into from Aug 16, 2018

Conversation

2 participants
@ry
Collaborator

ry commented Aug 13, 2018

cc @robbym

@ry ry requested a review from piscisaureus Aug 13, 2018

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 15, 2018

Collaborator

(Rebased to rw/flatbuffers@05e5943)

Collaborator

ry commented Aug 15, 2018

(Rebased to rw/flatbuffers@05e5943)

This was referenced Aug 15, 2018

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Aug 16, 2018

Collaborator

Why is this removed?

Collaborator

piscisaureus commented on js/main.ts in 29eb909 Aug 16, 2018

Why is this removed?

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 16, 2018

Collaborator

cmd id isn't used. I'm slowly extracting it. I'd rather it get put in when its used, if at all.

Collaborator

ry replied Aug 16, 2018

cmd id isn't used. I'm slowly extracting it. I'd rather it get put in when its used, if at all.

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Aug 16, 2018

Collaborator

All these unwraps are going to be a heache later when we start caring about security and not crashing on unexpected input.

Collaborator

piscisaureus commented on src/handlers.rs in 29eb909 Aug 16, 2018

All these unwraps are going to be a heache later when we start caring about security and not crashing on unexpected input.

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 16, 2018

Collaborator

I have a PR #529, several PRs from now, which adds proper error handling to all fo this.

Collaborator

ry replied Aug 16, 2018

I have a PR #529, several PRs from now, which adds proper error handling to all fo this.

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Aug 16, 2018

Collaborator

IMO it's not so logical to parse out the message here, would be better to have the handle_something() functions parse the message specific fields.
Of course fine for now.

Collaborator

piscisaureus commented on src/handlers.rs in 29eb909 Aug 16, 2018

IMO it's not so logical to parse out the message here, would be better to have the handle_something() functions parse the message specific fields.
Of course fine for now.

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 16, 2018

Collaborator

agree - noted.

Collaborator

ry replied Aug 16, 2018

agree - noted.

@ry ry merged commit b6912e7 into master Aug 16, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@ry ry deleted the rust_flatbuffers_upgrade branch Sep 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment