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

5.0.0 is not backwards compatible #59

Closed
donnfelker opened this issue Apr 4, 2022 · 2 comments · Fixed by #60
Closed

5.0.0 is not backwards compatible #59

donnfelker opened this issue Apr 4, 2022 · 2 comments · Fixed by #60

Comments

@donnfelker
Copy link

donnfelker commented Apr 4, 2022

Hey @haroldadmin I use your library and unfortunately the 5.0.0 changes are not fully backwards compatible.

If you have tests that use the code on the NetworkResponse model then your tests will break (as mine did) and you'll have to replace code with an actual Response instance.

Example (for those who run into this):

Pre 5.0.0 code with mockito:

  • Success:
    whenever(apiService.signOut(any())).thenReturn(NetworkResponse.Success(code = 200, body = Unit))

  • Error:
    whenever(apiService.signOut(any())).thenReturn(NetworkResponse.ServerError(code = 500, body = ErrorResponse("boom")))

New 5.0.0 Implementation:

  • Success:
    whenever(apiService.signOut(any())).thenReturn(NetworkResponse.Success(response = Response.success(200), body = Unit))

  • Error:

val errorResponse = Response.error<String>(500, "{\"error\": \"boom\"}".toResponseBody("application/json".toMediaTypeOrNull()))
whenever(apiService.signOut(any())).thenReturn(NetworkResponse.ServerError(response = errorResponse, body = ErrorResponse("boom")))

Note: toResponseBody and toMediaTypeOrNull are extension methods built into okhttp.

It would be good to include a CHANGELOG.md file in the repo with these notes (or somewhere accessile and easily found - I loo for a CHANGELOG.md file personally)

@haroldadmin
Copy link
Owner

Hey Donn! Thanks for reporting the issue.

You're right, this is a breaking change. I didn't expect consumers would manually instantiate NetworkResponse, which seems obvious in hindsight.

I'll create a CHANGELOG.md file with this info, and also update the Upgrade Guide.

@donnfelker
Copy link
Author

Awesome. Great to hear and thank you for the great lib!

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 a pull request may close this issue.

2 participants