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

[WIP] Move to OkHttp and add REST calls #26

Merged
merged 27 commits into from
Oct 10, 2017
Merged

[WIP] Move to OkHttp and add REST calls #26

merged 27 commits into from
Oct 10, 2017

Conversation

luciofm
Copy link
Contributor

@luciofm luciofm commented Sep 1, 2017

Work in Progress

Moving the SDK to OkHttp. Since we are going to use OkHttp for the REST calls, and we will use it on the App, doesn't make sense to have 2 websocket stacks consuming resources (size and methods).
Adding REST calls for various calls. On the new App, we will start to use REST calls on many methods, including login, list channels, list channel members, send messages, pin/unpin messages, star/unstar messages and more.

Use mockwebserver (from OkHttp) for testing the SDK

@sacOO7
Copy link
Contributor

sacOO7 commented Sep 1, 2017

Hey I'm not quite sure, how reliable OkHTTP websockets are. As far as REST is concerned, there are other libraries available which provides pure REST API's. I tested OkHTTP websockets and it gives problem for maintaining connection with server. For now lets do one thing, keep both libraries separate and test them on various platforms. I will create a separate release for SDK based on OkHTTP for native app, keep your branch sync with develop branch.

@sacOO7
Copy link
Contributor

sacOO7 commented Sep 1, 2017

I will test the current implementation for 2-3 days. I will see if it works great on various devices 👍 . Then I can merge it with develop. Till then try pushing your updates to OkHTTP branch.

@luciofm
Copy link
Contributor Author

luciofm commented Sep 1, 2017

This is still a working in progress, far from merge state yet.

@sacOO7
Copy link
Contributor

sacOO7 commented Sep 1, 2017

@luciofm still great work 👍

@sacOO7
Copy link
Contributor

sacOO7 commented Sep 8, 2017

Please do not change lots of method names. Users will need to alter their code if new artifact is published with changed method names. Try to update docs in sync with updated names.

@luciofm
Copy link
Contributor Author

luciofm commented Sep 12, 2017

@sacOO7 now is the time to break things... once we got a stable API and release 1.0, then we cannot break compatibility with previous versions...

@sacOO7
Copy link
Contributor

sacOO7 commented Sep 13, 2017

ohk, I think I need to test the API on your branch fully. Lot of code has been changed from last time.

@sacOO7
Copy link
Contributor

sacOO7 commented Sep 14, 2017

Hey please do not remove APIs for realtime, lets make them deprecated. Let's add a builder class for RocketChatAPI which can build both REST and realtime client (same as OkHTTP). I think it's better to separate both from each other. It's easy to manage that way and makes code more modular. I truly believe it is a better approach.

@sacOO7
Copy link
Contributor

sacOO7 commented Sep 14, 2017

Give it a new name like RocketChatRestClient. All rest APIs can reside in there. Maybe you can rename RocketChatAPI to RocketChatRealtimeClient (two different classes). That way code is more easy to handle and understand 👍 . Great work 💯

@sacOO7
Copy link
Contributor

sacOO7 commented Sep 15, 2017

Hey @luciofm please add your suggestions here

@sacOO7
Copy link
Contributor

sacOO7 commented Sep 15, 2017

Hey any thought on above suggestions? Please let me know about it. We can have a discussion on this.

- Rename LiveChatAPI to LiveChatClient
- Revert Callbacks to interfaces
- Rename some LiveChat listeners do Callbacks
- Moved some classes to an internal package
- removed old LiveChat tests - need to reimplement them based on WebsocketImplTest
Separate the implementation of REST api and Websocket Realtime API.
@sacOO7
Copy link
Contributor

sacOO7 commented Oct 4, 2017

I will add here points that will cover tasks that need to be covered before PR is merged.
I can see lot of code been changed. I think it is ok to say that every functionality needs to be covered that was available before changing the code.
E.g. Attachment types, DbManager, ConnectivityManager, reconnection, manual and auto reconnection etc

@sacOO7
Copy link
Contributor

sacOO7 commented Oct 4, 2017

I will add some code for LiveChat SDK that will cover changes to subscriptions middleware similar to core middleware.
I will also provide interface implementation for DbManager, so that it is configurable to store on disk by extending interface.

@sacOO7
Copy link
Contributor

sacOO7 commented Oct 4, 2017

TODO

  1. Working Attachment Models
  2. Interface implementation of DbManager (I think it should be present in websocketimpl since data will be stored from subscriptions which comes under realtime API)
  3. ConnectivityManager, reconnection, manual and auto reconnection are already available, just need accessor methods.
  4. All APIs to be well tested since models are available from new library, program may crash in runtime due to absense of particular key.
  5. Discussion related to abstract class callback, since I cannot implement it, I cannot have callback defined as a class member. If it is a interface, it introduces reusability by passing instance of a class as a callback inside holder.
  6. Make sure all models are correct and getting filled with all data that is being received from server.
  7. Merge ChatFactory and Improvements inside this PR and test each functionality. Refactor the code i.e. remove unnecessary code.
    **8. Advanced functionality : Add Task schedular to run certain tasks after reconnection
    i.e. subscriptions to rooms, authenticating again etc.
    Tasks can be assigned manually. Will be executed after each reconnection.

@luciofm
Copy link
Contributor Author

luciofm commented Oct 10, 2017

Merging to development. Still lots of stuff to break before 1.0

@luciofm luciofm merged commit 06cd6ca into develop Oct 10, 2017
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.

2 participants