Skip to content

Removed guava compile dependency#68

Merged
martijndwars merged 2 commits intoweb-push-libs:masterfrom
fischermatte:remove-guava
Sep 26, 2018
Merged

Removed guava compile dependency#68
martijndwars merged 2 commits intoweb-push-libs:masterfrom
fischermatte:remove-guava

Conversation

@fischermatte
Copy link
Contributor

  • removed guava, now using apache commons codec for base64 encoding (~320kb)
  • since webpush lib should stick to java 7, native base64 encoder of java 8 could not be used

@martijndwars I kept guava as testCompile dependency for now. just to make sure that there are non breaking changes with this pr (see Base64EncoderTest). but this could be removed as well. what do you think?

…ommons base64 encoding which is only 327kb.
@fischermatte fischermatte changed the title #66 removed guava compile dependency Removed guava compile dependency Sep 5, 2018
@martijndwars
Copy link
Member

Thanks! I think keeping Guava as a testCompile dependency is okay. I'll take a better look at the changes asap.

Our of curiosity: do you have a use-case in which the size of the dependency (Guava vs. commons codec) actually matters? Would it make sense to implement base64 encoding ourselves, and not rely on a dependency at all (e.g. to avoid licensing issues)?

@fischermatte
Copy link
Contributor Author

fischermatte commented Sep 7, 2018

do you have a use-case in which the size of the dependency actually matters

not really. i just stumbled across this because my deployment artifact grew quite a bit. among others i found this guava dependency (which i thought wasn't even used)

Would it make sense to implement base64 encoding ourselves

yes, prefered! i just wouldn`t spend much time on this. not better taking an existing one from some other os project? most newer are apache 2 licensed. does this work with MIT?

@fischermatte
Copy link
Contributor Author

just realized that commons-codec was already included as dependency - via apache-httpcomponents. I removed the explicit declaration from build.gradle. This means no additional jars with this pr...

@martijndwars martijndwars merged commit d6e7ccc into web-push-libs:master Sep 26, 2018
@martijndwars
Copy link
Member

Thanks! Merged!

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