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

Remove several dependencies #600

Merged
merged 4 commits into from
Feb 10, 2025
Merged

Conversation

patrikwlund
Copy link
Contributor

This library has a lot of dependencies for a library, several of which seem unnecessary.

Description

All applications depending on this library will transitively inherit all of its dependencies. The more libraries, the bigger the risk of conflicts, and also the bigger the potential attack surface (e.g. take-overs or sneaky PRs).

We use Vonage for its services, but we don't want to inherit random dependencies if we can avoid it. Especially small "unknown" ones.

I removed/replaced three libraries:

  • Epoch.net - easily replaced by DateTimeOffset.UtcNow.ToUnixTimeSeconds()
  • Yoh.Text.Json.NamingPolicies - does not seem to be in use
  • Handy.DotNETCore-Compatibility.ColorTranslations - i inlined the code

How Has This Been Tested?

The test suite passes in both NET 6 and NET 8.

@Tr00d
Copy link
Contributor

Tr00d commented Feb 10, 2025

Hi @patrikwlund,

Thanks for contributing. You'll find my thoughts below:

  • Epoch.net
    Indeed, that one can easily be replaced.

  • Yoh.Text.Json.NamingPolicies
    Indeed, that dependency stopped being used by end of 2023. Based on what I see, System.Text.Json introduced new JSON naming policies - specifically the one required (snake case). It's clearly a remnant that should be removed.

  • Handy.DotNETCore-Compatibility.ColorTranslations
    While I agree with the reason behind, I disagree with your proposal. You're basically putting the code from 'https://github.com/stephenhand/.NETCore-Compatibility-Helpers/blob/master/src/ColorTranslations/ARGB.cs' inside the codebase, with hardcoded colour codes which now become my responsiblity to maintain. I'm okay to get rid of that dependency, but not this way.
    Now, the ColorConverter is used in a single API... which is deprecated/obsolete and will be removed in the next major version. What I can do is try to get to that version faster and simply get rid of the dependency. The version didn't change since I started using the lib (0.2.1) but I can force that version to make sure there's no update until then.

@patrikwlund
Copy link
Contributor Author

Hi @patrikwlund,
While I agree with the reason behind, I disagree with your proposal. You're basically putting the code from 'https://github.com/stephenhand/.NETCore-Compatibility-Helpers/blob/master/src/ColorTranslations/ARGB.cs' inside the codebase, with hardcoded colour codes which now become my responsiblity to maintain. I'm okay to get rid of that dependency, but not this way.
Now, the ColorConverter is used in a single API... which is deprecated/obsolete and will be removed in the next major version. What I can do is try to get to that version faster and simply get rid of the dependency. The version didn't change since I started using the lib (0.2.1) but I can force that version to make sure there's no update until then.

I think you're worried for no reason since it hasn't had a need to change at all in the last 9 years, and it would just be internal code, but I think your suggestion is reasonable.

I reverted the relevant commit. Do you usually squash-merge, or would you like me to squash the commits beforehand?

@Tr00d
Copy link
Contributor

Tr00d commented Feb 10, 2025

It's a matter of months before the next major release, but I'll try to shrink that.
In the meantime, I'll create an issue on the repo to make sure it stays visible.

No worries, I'll squash it myself

@Tr00d Tr00d merged commit f4ece67 into Vonage:main Feb 10, 2025
4 checks passed
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