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

Add support for MsgPack #161

Merged
merged 9 commits into from
Jun 23, 2022
Merged

Add support for MsgPack #161

merged 9 commits into from
Jun 23, 2022

Conversation

AmirQSD
Copy link

@AmirQSD AmirQSD commented Jun 10, 2022

I had to alter a few tests that included base64 decode, since with messagepack, binary data is sent as bytes, without the need to specify the base64 encoding.

@AmirQSD AmirQSD self-assigned this Jun 10, 2022
@qsdstefan qsdstefan changed the title Add support for MssgPack Add support for MsgPack Jun 10, 2022
@qsdstefan
Copy link

qsdstefan commented Jun 10, 2022

Looks good for now, but you will need to update .github/workflows/check.yml by adding protocol: [ 'json', 'msgpack' ] to the matrix, then under env you should pass PROTOCOL as PROTOCOL: ${{ matrix.protocol }} to provide the environment variable in the final workflow step which is Run test suite
See check.yml#L14 for details

Copy link

@qsdstefan qsdstefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As previously stated, update .github/workflows/check.yml to run tests for binary protocol version.

Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work 🙂

Couple of formatting fixes to be made, my main comment however is that according to RSC8a msgpack should be the default as long as PHP as a platform has sufficient support for it. I haven't looked at rybakit/msgpack but are you able to comment on whether it should work fine for any PHP application (ie does it require any system dependencies that weren't already required by ably-php)? If it's not gonna be a breaking change then I think we should change the default useBinaryProtocol client option to true.

@AmirQSD AmirQSD requested review from qsdstefan and owenpearson June 15, 2022 14:24
Copy link

@qsdstefan qsdstefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good after the changes to make MsgPack the default.

Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks 👍

@sacOO7
Copy link
Collaborator

sacOO7 commented Jun 20, 2022

Other than few changes, looks good to me overall 👍

@AmirQSD AmirQSD requested a review from sacOO7 June 20, 2022 15:47
Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@owenpearson owenpearson merged commit d3b9bda into main Jun 23, 2022
@owenpearson owenpearson deleted the Add_support_for_MssgPack branch June 23, 2022 13:25
@AmirQSD AmirQSD mentioned this pull request Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants