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

Declare base64 as a dependency #30

Closed
wants to merge 1 commit into from
Closed

Declare base64 as a dependency #30

wants to merge 1 commit into from

Conversation

Earlopain
Copy link

base64 will be removed from the default gems in Ruby 3.4, see rack/rack#2110

Since this uses base64 in more than just one place I created a module containing the needed methods instead. That way usage is clear and easy to follow.

Because this basically copies the whole of https://github.com/ruby/base64/blob/master/lib/base64.rb (sans docs) I have included their license in the file. I wouldn't have bothered for just the pack/unpack wrappers but the url encode/decode methods are a bit more involved.

The newrelic gem is one where I have taken the similar approach to this if you are interested: newrelic/newrelic-ruby-agent#2378. In most other gems I was just able to do the same thing as in rack itself.

@dentarg
Copy link
Contributor

dentarg commented Jan 16, 2024

This is starting to get crazy, why not eat the dependency?

@Earlopain
Copy link
Author

🤷 I've made PRs to remove base64 from about a dozen projects by now that are in use just by one of my rails projects. Most have been more trivial though, like the PR in rack.

Removing this from ruby itself seems like a mistake. It's such basic functionality that I'm flabbergasted that there are no methods for it on String or something that are just available (maybe something like the base64 gem -winkwink-). What's even better is that securerandom provides base64 method for generation itself like base64 or urlsafe_base64 while still being bundled. Whatever, not my thing to decide.

I would prefer not relying on base64 for such trivial functionality in half my gems I depend on, though I have already improved the situation there in other gems already. In the end its your decision how to handle this.

@dentarg
Copy link
Contributor

dentarg commented Jan 16, 2024

I'm not the maintainer of rack-session so it is not my decision to make here, but for the gems I maintain, I have chosen to rely on the dependency.

I can see the point of inlining when it is just one or two #unpack/#pack calls, but copying all of it seems to run counter to what the Ruby project tries to achieve with making base64 a bundled gem – to be able to release security patches more quickly and independent of Ruby. Who will maintain these vendored copies going forward? I suspect it will not happen or be unnecessary work for already busy maintainers.

@dentarg
Copy link
Contributor

dentarg commented Jan 16, 2024

@Earlopain Do you not want to have the base64 gem as a dependency in your Rails application? If so, is there a reason for that?

@Earlopain
Copy link
Author

Earlopain commented Jan 16, 2024

I just like to have a small dependency graph. Relying on a dependency for just a single method like in racks example seems like something that I'd see in JavaScript. In my uneducated opinion security concerns are non-existant for this implementation.

I understand that its a bit more complicated here and I'm somewhat happy either way if rack-session doesn't break in the future with Ruby 3.4. If so, maintainers here can feel free to close and just add it to the gemspec.

There is also sidekiq/sidekiq@49389da that sidekiq did by simplifying the implementation of the urlsafe methods which would bring it closer to just being a wrapper again. Though I don't know enough about internals here to say if that is fine to do.

@dentarg
Copy link
Contributor

dentarg commented Jan 17, 2024

🤷 I've made PRs to remove base64 from about a dozen projects by now that are in use just by one of my rails projects. Most have been more trivial though, like the PR in rack.

I just like to have a small dependency graph.

I guess you aren't running Rails 7.1 yet? It will bring base64 as a dependency via activesupport: rails/rails#48907

@Earlopain Earlopain changed the title Remove runtime dependency on base64 Declare base64 as a dependency Jan 17, 2024
@Earlopain
Copy link
Author

I'm aware, yes. I haven't tried make a PR there, a bit hypocritical if you ask me.

Anyways, not everyone uses rails with rack so relying on that on pulling in the dependency doesn't work. They might also decide to remove it in the future. mutex_m for example has been replaced after that PR already.

I added it to the gemspec here instead as well.

@ioquatix ioquatix closed this in #31 Jan 17, 2024
@ioquatix
Copy link
Member

Thanks for the great discussion. I don't think we want to rewrite base64 here.

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