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

Rails 7.0 support and drop older railses #63

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

casperisfine
Copy link
Contributor

As well as remove unused features (e.g. read only).

This if merged should be a major bump.

@rafaelfranca
Copy link
Member

What happens if someone report a bug with Rails 6.1 with the actual released branch? We create a release branch for the current major?

@casperisfine
Copy link
Contributor Author

What happens if someone report a bug with Rails 6.1 with the actual released branch?

We can maintain a 2.x-stable branch. But based on the download numbers, I doubt there's many non-Shopify users out there: https://rubygems.org/gems/memcached_store, maybe a few but with the state of memcached and memcached_store I'd be really surprised.

@casperisfine casperisfine force-pushed the rails-7.0-support branch 2 times, most recently from 1a157d5 to cdf5262 Compare July 22, 2021 08:43
Copy link
Member

@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

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

LGTM

@serializer = serializer
@compressor = compressor
end
attr_accessor :swallow_exceptions
Copy link
Member

Choose a reason for hiding this comment

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

We should remove it since we don't support it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@shioyama
Copy link
Member

shioyama commented Aug 2, 2021

This LGTM but I haven't been able to fully follow the corresponding Rails changes, so I'm holding off reviewing yet. I'll focus on decoupling our code from this gem altogether where it makes sense so changes here have less impact.

Copy link
Member

@shioyama shioyama left a comment

Choose a reason for hiding this comment

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

I'm just commenting for now, I'll try creating a failing test to check what I commented.

Entry.new(payload, compress: false)
end
else
super(payload)
Copy link
Member

Choose a reason for hiding this comment

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

I think this will break raw values read back from the cache.

Currently a raw value written to the cache with Rails.cache.write("foo", "bar", raw: true) can be read back with just Rails.cache.read("foo") (no raw: true), but with this change this won't work unless you pass raw: true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I broke this knowingly.

Copy link
Member

@shioyama shioyama left a comment

Choose a reason for hiding this comment

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

This fails:

  def test_raw
    assert @cache.write("foo", "1", raw: true)
    assert_equal "1", @cache.read("foo")                                                                                                                          
  end

It seems like all tests on raw values now pass raw: true to read / fetch, so maybe this is intentional? But if so it will break a lot of stuff, and I'm not even sure how it could be handled in contexts where you're not sure if the stored value is raw or not.

Copy link
Member

@shioyama shioyama left a comment

Choose a reason for hiding this comment

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

Approving since the breaking change is known, although I'm not sure how it should be handled.

@casperisfine
Copy link
Contributor Author

if so it will break a lot of stuff,

You think? Shouldn't be too hard to track down no?

@casperisfine casperisfine force-pushed the rails-7.0-support branch 2 times, most recently from 2f73343 to 1cb8312 Compare August 17, 2021 15:28
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.

5 participants