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

Allow configuring a serializer and support binary data #56

Conversation

pojiro
Copy link
Contributor

@pojiro pojiro commented May 15, 2023

This PR tries to implement handling binary data feature, relates to #28 and #53.

This may include implementation and testing omissions due to a lack of understanding of the Slipstream overview.

Roughly, I first prepare the Serializer behaviour as Slipstream.Serializer, then implement client side silializer which corresponding to Phoenix V2 JSON serializer.

I followed the TDD, first added tests to synchronicity_test.exs and then implemented to satisfy them.

This PR is intended to be the base of implementation and should be modified to fit this project.

@pojiro pojiro marked this pull request as ready for review May 15, 2023 08:52
@pojiro pojiro changed the title Add trial implementation of handling binary data feature [WIP] Add trial implementation of handling binary data feature May 15, 2023
@pojiro
Copy link
Contributor Author

pojiro commented May 15, 2023

Currentry I refactor lib/slipstream/connection/impl.ex, could you wait a little longer for the review, till it will be finished.

finished.

@pojiro pojiro force-pushed the trial-implementation-of-handling-binary-data branch from 06d1449 to 776cb4b Compare May 15, 2023 11:37
@pojiro pojiro changed the title [WIP] Add trial implementation of handling binary data feature Add trial implementation of handling binary data feature May 15, 2023
@pojiro
Copy link
Contributor Author

pojiro commented May 17, 2023

@the-mikedavis Could you review this when you are free?

@the-mikedavis
Copy link
Collaborator

Thanks for the PR! I am away for a few weeks but I will give the changes a proper look when I get back.

@pojiro
Copy link
Contributor Author

pojiro commented Jun 4, 2023

I fixed the credo issue. I will fix the following coverall issue later, thanks.

https://coveralls.io/builds/60510158/source?filename=lib%2Fslipstream%2Fserializer%2Fphoenix_socket_v2_serializer.ex

@pojiro
Copy link
Contributor Author

pojiro commented Jun 5, 2023

Coverall issues are fixed, thanks.

Copy link
Collaborator

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

This looks great! The only thing that worries me is the change from json_parser to serializer for configuration: that's a breaking change so we'd need to cut a 2.0.0 release. Instead could we keep the json_parser option around and mark it as deprecated using the :deprecated NimbleOptions option and use that as a default value in Slipstream.Serializer.PhoenixSocketV2Serializer rather than hard-coding Jason?

Alternatively we could keep the json_parser option around without deprecating it so that it would be easy to switch out that module without re-implementing the serializer, and document that it has no effect if using a custom serializer

@the-mikedavis the-mikedavis linked an issue Jun 6, 2023 that may be closed by this pull request
@pojiro
Copy link
Contributor Author

pojiro commented Jun 6, 2023

Thanks for your review! I added one commit for it. This commit make that

  • user can change json_parser as before
  • there is no need to deprecate json_parser config

How about this?

@pojiro pojiro force-pushed the trial-implementation-of-handling-binary-data branch from a065394 to d7080f5 Compare June 6, 2023 23:14
json_parser: [
doc: """
A JSON parser module which exports at least `encode/1` and `decode/2`.
A JSON parser module which exports at least `encode!/1` and `decode!/1`.
Copy link
Contributor Author

@pojiro pojiro Jun 6, 2023

Choose a reason for hiding this comment

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

This makes a breaking change for users who use custom JSON parser which doesn't have decode!/1.

before this PR

The original line is not fit the actual codes. The codes expected encode!/1 and decode/1. This was just document error. Custom JSON parser users know that and implement them to fit the codes.

after this PR

This PR codes expect decode!/1 for json_parser. So this change should be written in CHANGELOG.

@pojiro pojiro force-pushed the trial-implementation-of-handling-binary-data branch from d7080f5 to 14d1731 Compare June 6, 2023 23:40
@pojiro pojiro requested a review from the-mikedavis June 6, 2023 23:41
Copy link
Collaborator

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for working on this!

@the-mikedavis the-mikedavis changed the title Add trial implementation of handling binary data feature Allow configuring a serializer and support binary data Jun 9, 2023
@the-mikedavis the-mikedavis merged commit a123492 into NFIBrokerage:main Jun 9, 2023
3 checks passed
@pojiro pojiro deleted the trial-implementation-of-handling-binary-data branch June 9, 2023 22:36
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.

Allow pushing binary data
2 participants