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

Replace Moshi with generated marshaller for operation json serialization #578

Merged
merged 8 commits into from
Jul 20, 2017

Conversation

sav007
Copy link
Contributor

@sav007 sav007 commented Jul 19, 2017

Part of #572

Next step is to remove Moshi from SqlNormalizedCache

@digitalbuddha
Copy link
Contributor

@sav007 should this be made configurable. I worry that most won't care about the optimization but will care about the additional methods the request parsing generates.

@sav007
Copy link
Contributor Author

sav007 commented Jul 19, 2017

@digitalbuddha well I see your point but not sure if that won't be a messy to have it both. I fell like we already have too much configs, and it was our initial idea use generation instead of Moshi.
Plus I feel like we really won't benefit of saving couple methods here. Maybe we can later optimize it a little bit when such concern raised.

@digitalbuddha
Copy link
Contributor

digitalbuddha commented Jul 19, 2017 via email

@sav007 sav007 merged commit 54d5266 into apollographql:master Jul 20, 2017
@sav007 sav007 deleted the feature-572/replace-moshi branch July 20, 2017 23:03
@jaredsburrows
Copy link
Contributor

@digitalbuddha I completely agree.

@sav007 In this PR, I did not see you remove the dependency from the build.gradle. Did you remove it in another PR?

@jaredsburrows
Copy link
Contributor

Are you going to remove moshi completely from this repository? It looks like it is still used: https://github.com/apollographql/apollo-android/search?utf8=%E2%9C%93&q=moshi&type=.

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