Relax gem requirement to allow Rails 7

[jpawlyn]

Purpose

Follow on from #2423 to enable AMS to be used with Rails 7

Changes

Updates rails_versions in Gemspec to < 7.1

Caveats

Related GitHub issues

Additional helpful information

[delphaber]

Since Ruby 3.0 is "preferred" in Rails 7, should Ruby 3.0 be added to travis too? 🤔

[wasifhossain]

I think AMS is currently using Github CI instead of Travis. could you please make the changes on https://github.com/rails-api/active_model_serializers/blob/0-10-stable/.github/workflows/ci.yml.

Thanks!

[wasifhossain]

as the rails version on main branch is now having 7.1.0, can we use < 7.1 instead of < 8.0. thanks

[jpawlyn]

Good idea

[bf4]

might as well remove the max version. only real benefit is it gets people to ask us to add a new version to CI

[jpawlyn]

@wasifhossain had a different idea but happy to go with the consensus.

[NickLaMuro]

Thought I should throw a couple of relevant links in here since this took me a bit too stumble across this PR and almost opened a duplicate PR:

A big reason most will need this fix pretty quickly is because without it, you will get forced into using an ancient version of the gem predating this change:

#1736

This means that the errors mentioned in and fixed by #2395 are not in the gem version that is bundled, leading to a very confusing upgrade experience.

(Edit: This assumes a loose gem dependency to this gem in the Gemfile of the Rails project, which maybe arguably should be fixed in the project I am working on... 😖 )

For now, using the jpawlyn:0-10-stable branch for this should be enough for proof of concept purposes, but is probably not best for prime time.

---

Thanks for all the hard work and will be looking forward to this getting merged. 🙇

[wasifhossain]

my only concern is, whether we should go forward and adopt the new rails practice (resetting the instance variables) by rewriting the affected tests in this repo, or should we pull things behind by using this hack

[jpawlyn]

One other option is to target the hack at the one controller that causes the issue ImplicitSerializationTestController in order to limit the likelihood of instance variables being re-used across requests in future.

ie

# frozen_string_literal: true

require 'test_helper'

module ActionController
  module Serialization
    class ImplicitSerializerTest < ActionController::TestCase
      class ImplicitSerializationTestController < ActionController::Base
        include SerializationTesting
        
        ...

        # HACK: to prevent the resetting of instance variables after each request in Rails 7
        # see https://github.com/rails/rails/pull/43735
        def clear_instance_variables_between_requests; end

        def update_and_render_object_with_cache_enabled
          @post.updated_at = Time.zone.now # requires hack above to prevent `NoMethodError: undefined method `updated_at=' for nil:NilClass`

          generate_cached_serializer(@post)
          render json: @post
        end

        ...

[wasifhossain]

I think that would be a more optimized approach. this way we can apply the hack on the affected controllers exclusively, at the same time influence adopting the new practice before writing new tests.

Also in future iteration, we may proactively work on getting rid of this hack from these controllers by rewriting the tests.

[wasifhossain]

Hi @jpawlyn the PR looks good to me! thanks for your efforts 👍

as ruby 3.1.0 has just been out, would you mind adding this in github CI as well 🙂 thanks!

[jpawlyn]

Hi @jpawlyn the PR looks good to me! thanks for your efforts 👍

as ruby 3.1.0 has just been out, would you mind adding this in github CI as well 🙂 thanks!

Sure, easily done.

[jpawlyn]

Hi @jpawlyn the PR looks good to me! thanks for your efforts 👍
as ruby 3.1.0 has just been out, would you mind adding this in github CI as well 🙂 thanks!

The CI error is due to an incompatibility I think between Rails 7.0 and ruby 3.1 (see rails/rails#43998). It has been fixed on main (rails/rails#43951) so once Rails 7.0.1 is released it should be fine.

[bf4]

I figure this is fine to merge as it's going to keep failing on Rails 7.0 and Ruby 3.1 due to incompatibilities outside of AMS scope?

[jpawlyn]

I figure this is fine to merge as it's going to keep failing on Rails 7.0 and Ruby 3.1 due to incompatibilities outside of AMS scope?

I reckon so 👍🏼

[wasifhossain]

Great! let's merge it.

[AhmedKamal20]

Will we get a new release?

[bopm]

@bf4 I am going to tag you here, as this seems to be stalled for a week.

[bf4]

I just released 0.10.13. I didn't check to make sure the change log is up to date. Help appreciated.