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 user to add custom codecEngine #27

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AUT0CRAT
Copy link

PR for #10
A basic implementation of Binary Codec for JAVA client.

@sacOO7
Copy link
Owner

sacOO7 commented Mar 22, 2018

Hi @AUT0CRAT , will take a look at PR and get back to you 👍

@XDex
Copy link
Contributor

XDex commented Mar 26, 2018

@AUT0CRAT Could you please provide an example of how the codec is used, possibly adding some testing code to Main class?
I'm not really sure the codec will work as intended, not with the current Socket implementation supporting only the NV client's WebSocket.sendText() and converting the JSON object to String before sending it..
I think support for binary WebSocket frame sending should be implemented first, prior to adding SC codecs support.. Btw, I'm planning on doing it soon..

@sacOO7
Copy link
Owner

sacOO7 commented Mar 26, 2018

Hi @AUT0CRAT , it will be helpful if you can provide working example. I will try to integrate it by trying out send method with binary data.


List<Object> a = new ArrayList<>();
a.add(dataObject.optString("channel"));
a.add(dataObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be fixed as: a.add(dataObject.optJSONObject("data"));

@sacOO7
Copy link
Owner

sacOO7 commented Apr 3, 2018

Hi @XDex , if you know how to use code defined in PR, it will be useful if you can post working example here 👍 or create PR for it.

@XDex
Copy link
Contributor

XDex commented Apr 3, 2018

@sacOO7 Actually, I'm now working on adding support for sc-min-bin codec in my own fork, however I have refactored it to work with Jackson instead of org.json JSON serializer, as implementation is much simpler with jackson-dataformat-msgpack MsgPack plugin.
I can raise a PR once I'm done, however I guess many people would prefer to have a more lightweight implementation without binary codecs support, so maybe it makes more sense to publish it as a separate project, WDYT?

@sacOO7
Copy link
Owner

sacOO7 commented Apr 5, 2018

Hi @XDex , I don't think making a separate project a good idea. We have already decided to keep one client per language (since too many clients is creating problem for users to choose one) . It's better if we can keep it at one place. It's easy to maintain that way, and also people can raise and find issues at one place. I don't think 3-4 class files for managing codecs will make implementation heavy as a library ..

@XDex
Copy link
Contributor

XDex commented Apr 5, 2018

@sacOO7 Generally I agree with your point, however it's always better to offer users choice when the differences are too big and breaking, which is the case here.. I've noted the differences here, and unfortunately they are quite impacting: ~150 Kb JAR vs ~1.8 Mb, plus breaking API changes..

@sacOO7
Copy link
Owner

sacOO7 commented Jun 25, 2018

HI @XDex , do you have any plans to release library with sc codec min? I mean we can conclude on solution which will work for us... Even keeping the library size as small as possible...

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