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

Change base64 crate to simple-base64 crate #356

Closed
wants to merge 1 commit into from

Conversation

drogus
Copy link
Collaborator

@drogus drogus commented Oct 3, 2023

Description of Changes

Standard base64 encode/decode functions have been deprecated and it's clear that the maintainer doesn't care about ergonomics of using the crate[1], so I've forked the crate to be able to get back to a simple format of calling the decode/encode functions.

API and ABI

  • This is a breaking change to the module ABI
  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

Standard base64 encode/decode functions have been deprecated and it's
clear that the maintainer doesn't care about ergonomics of using the
crate[1], so I've forked the crate to be able to get back to a simple
format of calling the decode/encode functions.
Copy link
Contributor

@kulakowski kulakowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you when to rebase and land this

@@ -14,7 +14,7 @@ spacetimedb-client-api-messages = { path = "../client-api-messages", version = "

anyhow.workspace = true
anymap.workspace = true
base64.workspace = true
simple-base64.workspace = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reorder to alphabetical please.

@@ -26,7 +26,7 @@ spacetimedb-client-api-messages = { path = "../client-api-messages", version = "
anyhow.workspace = true
async-trait.workspace = true
backtrace.workspace = true
base64.workspace = true
simple-base64.workspace = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@kulakowski kulakowski added the release-any To be landed in any release window label Oct 27, 2023
Copy link
Contributor

@kim kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this. The only reason to move away from the base64 crate seems to be a convenience function, which we could easily define in lib or wherever. I am fairly sure that we would depend on the original base64 crate regardless, even if transitively.

@kulakowski
Copy link
Contributor

I disagree with this. The only reason to move away from the base64 crate seems to be a convenience function, which we could easily define in lib or wherever. I am fairly sure that we would depend on the original base64 crate regardless, even if transitively.

I don't feel strongly. I'll clarify, though, that simple-base64 is a fork of the original base64 and not a wrapper. So there would not be a transitive dependency on it.

@kim
Copy link
Contributor

kim commented Oct 30, 2023

simple-base64 is a fork of the original base64 and not a wrapper.

Well, that's my point. Because base64 is the obvious choice for the purpose, it is quite likely to be re-introduced either by us or transitively -- which will just make a future audit of our dependency footprint harder.

There are surely reasons to avoid certain crates even if "obvious choice", but then we should ensure that we avoid them entirely (e.g. via cargo deny) and provide ample documentation about what to use instead and why.

I just don't think this is a good reason, because we could solve the perceived nuisance with a wrapper.

@kulakowski
Copy link
Contributor

simple-base64 is a fork of the original base64 and not a wrapper.

Well, that's my point. Because base64 is the obvious choice for the purpose, it is quite likely to be re-introduced either by us or transitively -- which will just make a future audit of our dependency footprint harder.

There are surely reasons to avoid certain crates even if "obvious choice", but then we should ensure that we avoid them entirely (e.g. via cargo deny) and provide ample documentation about what to use instead and why.

I just don't think this is a good reason, because we could solve the perceived nuisance with a wrapper.

Ahh I understand now, I misunderstood and thought you had the specific objection that the new crate itself transitively depended on it.

Yeah. I could see adopting this if other people do, given that? I don't feel as against it as Kim does but certainly see the line of reasoning.

@kulakowski
Copy link
Contributor

@drogus any thoughts about the above discussion?

@kulakowski
Copy link
Contributor

This has been open for a while with unaddressed feedback. I'm going to close it for the sake of making the release process easier (the fewer PRs to scan the easier). Please reopen it if we need to, with the feedback addressed.

@kulakowski kulakowski closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants