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 digest dependency from gemspec #202

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

iMacTia
Copy link
Contributor

@iMacTia iMacTia commented May 23, 2022

What are you trying to accomplish?

digest is a standard gem and as such is bundled with Ruby 3.0+, while in previous Ruby versions it's part of stdlib.
Having the gem explicitly added to the gemspec can cause issues downstream on projects using older ruby versions.
This is because it's actually possible to load the code twice, once from the stdlib and once from the gem.

As a reminder, the dependency was added as a result of this comment.

What approach did you choose and why?

Removing the dependency should have absolutely no effect on the code.
For people running Ruby 2.7 or previous, the require "digest" in cache.rb will load the stdlib file, while for those running Ruby 3.0+ it will load the standard gem.

What should reviewers focus on?

Cross-compatibility with Ruby 2 and Ruby 3

Type of Change

  • Bugfix
  • New feature
  • Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Checklist

  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • It is safe to rollback this change.

cc @rafaelfranca @alexevanczuk

@iMacTia iMacTia requested a review from a team as a code owner May 23, 2022 09:32
@ghost ghost added the cla-needed label May 23, 2022
@iMacTia
Copy link
Contributor Author

iMacTia commented May 23, 2022

CLA is signed, not sure how to re-run the CI now though 😄

@alexevanczuk
Copy link
Contributor

This looks fine to me, thanks @iMacTia !

@ghost ghost removed the cla-needed label Jun 7, 2022
@gmcgibbon
Copy link
Member

while for those running Ruby 3.0+ it will load the standard gem.

> gem uninstall digest
Gem digest-3.1.0 cannot be uninstalled because it is a default gem

Hm, so digest is a default gem that can't be uninstalled. TIL. Seems kind of confusing that you can load default gems twice, but I guess Ruby 2 has no knowledge of this new paradigm and since the gemspec of digest supports Ruby 2, it just installs it like any other gem.

Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Thanks!

@gmcgibbon gmcgibbon merged commit d1e4a38 into Shopify:main Jun 8, 2022
@iMacTia iMacTia deleted the mg/remove-digest-dependency branch June 9, 2022 08:53
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems July 12, 2022 00:06 Inactive
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.

None yet

3 participants