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 support? #385

Closed
mmhan opened this issue Oct 8, 2021 · 30 comments
Closed

Rails 7 support? #385

mmhan opened this issue Oct 8, 2021 · 30 comments

Comments

@mmhan
Copy link

mmhan commented Oct 8, 2021

Given than rails 7 has released an alpha version, is rails 7 support coming to simple_token_authentication soon?

@mmhan
Copy link
Author

mmhan commented Oct 30, 2021

I'm interested to contribute to this repo by resolving this issue. However, this will be the first time I will be opening a PR on github and I'm in need of some guidance on how to proceed.

So far, I have forked the repository, cloned it and have it run existing tests using rake spec with success.

As the next step, I generated new config for appraisal. Note: we need to use the main branch of devise for rails 7 compatibility.

appraise 'rails_7_devise_4' do
  gem "actionmailer", "~> 7.0.0.alpha2"
  gem "actionpack", "~> 7.0.0.alpha2"
  gem "activerecord", "~> 7.0.0.alpha2"
  gem "devise", git: 'https://github.com/heartcombo/devise'
end

With that done, I found that mongoid is breaking on rails 7 too with the error. uninitialized constant ActiveModel::Serializers::Xml This is where I am stuck at because mongoid's JIRA does not have any related issue about rails 7.

Please let me know how best I can help to move this forward.

@amorimlucas
Copy link

We need this done too. Is there a maintainer you could contact directly @mmhan?

@OskarEichler
Copy link
Contributor

@gonzalo-bulnes Us too 👍🏼

@OskarEichler
Copy link
Contributor

@mmhan Maybe try again now that Rails 7 is officially released. Also found this:

https://stackoverflow.com/questions/43254528/nameerror-uninitialized-constant-activemodelserializersxml-when-declaring-a

Might need to add those as dependencies - not sure but worth a shot:

gem 'activemodel-serializers-xml'
gem 'active_model_serializers'

@gonzalo-bulnes
Copy link
Owner

gonzalo-bulnes commented Dec 19, 2021

Hi @mmhan,

Opening a PR is the right way to go! ❤️
It has happened in the past that Mongoid support for new versions of Rails took a bit of time to be realeased, and we've worked around that (the linked commit was the resolution of the workaround).

Note that as long as Devise's support for Rails 7 is not released, we'll have to wait. This gem relies heavily on Devise, and relying on unstable versions of Devise is not an option. Don't let that prevent you from preparing your PR from Devise's main branch! Eventually, it will get released 🙂

With regards to @OskarEichler suggestion: I haven't looked at the situation in detail, but some parts of Rails are sometimes extracted to gems. That is usually documented in the Rails migration docs from one version to another.

@gonzalo-bulnes
Copy link
Owner

gonzalo-bulnes commented Dec 19, 2021

See also: #367 on the topic of Mongoid support.

@gonzalo-bulnes
Copy link
Owner

You may already have seen them @mmhan, but the updates related to the two previous major Rails releases are a good reference to get an idea of what adding support for a new version of Rails looks like:

Add Rails 5 support: #262
Add Rails 6 support: #349

@OskarEichler
Copy link
Contributor

@gonzalo-bulnes Thanks so much for looking into this! As far as I can tell the main branch of Devise is the stable branch - the older stable branches haven't been updated in years, and the main branch is very actively maintained.

Maybe worth opening an issue and asking if the main branch can be considered stable and used. Will do that right now ;)

@gonzalo-bulnes
Copy link
Owner

gonzalo-bulnes commented Dec 22, 2021

Note @OskarEichler that even if the main Devise branch is actively maintained (I have all reasons to think it is!) doesn't make it more fit to be referenced as a dependency. Let me explain.

When any dependency is updated, it is possible that its changes are incompatible with the way you use them (depend on them). If that were to happen, any installation of your gem would stop working the way it is expected (which may or not be obvious, but could be hard to detect).

Usually, with well-maintained gems, like Devise, the features that are dependable are documented, and their maintainers communicate when breaking changes happen by choosing carefully the new release number (see semantic versioning).

If any gem doesn't reference its dependencies by release number... well, it cannot take advantage of that information. 🤷‍♀️ On the contrary, if a gem references its dependencies by release number, when a new release breaks important assumptions nothing happens, because the previous release remains in use until explicitly updated. 🥇

That is what I meant when I said "unstable" - if it was based on the main label, the relationship between both gems would be unstable: the assumptions on which it is built could change without notice. In other words Devise vX.Y.Z will always have the same set of features it had on release day, but Devise main features change all the time by design. 🙂

Summary: Devise will eventually create a release that supports Rails 7, until then, we're not ready to support it either! Does this make sense?

@OskarEichler
Copy link
Contributor

OskarEichler commented Dec 22, 2021

Hey @gonzalo-bulnes, thanks so much for the long explanation - makes total sense.

They did release version 4.8.1 with Rails 7 support on the main branch (here)- why not just lock in the dependancy at that version?

@gonzalo-bulnes
Copy link
Owner

gonzalo-bulnes commented Dec 22, 2021

Great @OskarEichler, that solves the Devise requirement. 4.8.1 is within the existing version constraints for Devise, which means that Devise 4.8.1 can already be used with this gem, and there isn't anything special that need to be done for that.

On the testing side of things, now the appraisal that @mmhan got started (above) can be updated to reference Devise 4.8.1 ✅ (Appraisals allow to test specific combinations of dependencies together.)

Then will likely remain to follow up on Mongoid in the same way 🙂

@mmhan
Copy link
Author

mmhan commented Dec 22, 2021

I'm afraid things are a little too much at home right now with all the activities related to settling in after recently migrating from one country to another. I will get back to this once all the dust has settled. Feel free to take over from here if anyone wishes to. If not, I'll drop a comment when I can get around to it.

@gonzalo-bulnes
Copy link
Owner

No rush on my side! Thanks for following up @mmhan (and sharing your progress in the first place!)

It seems @OskarEichler is on a good track, and maybe this will incentive others to join the thread 🙂

@OskarEichler
Copy link
Contributor

OskarEichler commented Dec 22, 2021

@gonzalo-bulnes Okay I tested all of this, thanks for the help here @mmhan

The issue this is failing is because mongoid hasn't been updated to support Rails 7 yet, see here:
mongodb/mongoid#5107

When running Appraisal it actually resolves mongoid to an old beta version (4.0.0.beta2) because that probably didn't have a gemspec. It's not compatible and once forcing the latest version of Mongoid by specifying it in the Appraisal file the bundler fails because it is not yet compatible:

appraise 'rails_7_devise_4' do
  gem "actionmailer", "~> 7"
  gem "actionpack", "~> 7"
  gem "activerecord", "~> 7"
  gem "devise", git: 'https://github.com/heartcombo/devise'
  gem "activemodel-serializers-xml"
  gem "active_model_serializers"
  gem "mongoid", "7.3.3"
end

Will need to wait until Mongoid is updated...

@OskarEichler
Copy link
Contributor

@gonzalo-bulnes Actually it seems like mongoid is only a dependancy for development:

s.add_development_dependency 'mongoid', ">= 3.1.0", "< 8"

Can this potentially be ignored or why do you need it in development mode?

@amorimlucas
Copy link

amorimlucas commented Jan 16, 2022

Looks like Mongoid was updated to support Rails 7 a few days ago. Was that the only thing holding this up?

@OskarEichler
Copy link
Contributor

@amorimlucas Just updated Mongoid and looks like everything is working now indeed!

@gonzalo-bulnes Made a PR here for you to review and merge: #391

@amorimlucas
Copy link

@gonzalo-bulnes Any progress on this? Is there someone else that could make this merge? Thx

@iGEL
Copy link

iGEL commented Feb 14, 2022

@OskarEichler Thank you!
@gonzalo-bulnes Also thank you for maintaining this software. #391 seems to be reasonable to me and is quite small. Any chance you could review it soon and release a new version, if it's ok? It's blocking our Rails 7 upgrade.

@GregSmith92
Copy link

GregSmith92 commented Apr 25, 2022

Just wanted to check in and see if #391 is any closer to being merged? 🙏 🙏 🙏

@aried3r
Copy link

aried3r commented May 17, 2022

Using #391 as a base, I ran the tests using appraisal for Rails 5, 6 and 7 using Ruby 2.5, 2.7, 3.0 and 3.1 after relaxing the development dependency in the .gemspec file (see https://github.com/gonzalo-bulnes/simple_token_authentication/pull/391/files#r874883121).

Ruby 2.5 (except with Rails 7, which only supports Ruby 2.7+) and 2.7 complete successfully, but Ruby 3.0 and 3.1 have three test failures on all Rails versions. All three are almost identical failures and while I haven't looked into them yet, I believe it's because of Ruby 3.0+'s kwarg handling, but I could be wrong.

Also I don't think these are related to Rails 7 support so are not necessarily blocking.

The test failures looks like this:

  1) Simple Token Authentication :sign_in_token option can be modified from an initializer file
     Failure/Error: controller.send(:sign_in, record, *args)

       #<#<Class:0x0000000113d93050>:0x0000000113d81c60> received :sign_in with unexpected arguments
         expected: (#<Double (anonymous)>, {:store=>"updated value"})
              got: (#<Double (anonymous)>, {:store=>"updated value"})

@gonzalo-bulnes, would you be open to a little cleanup of supported Ruby and Ruby on Rails versions? Rails 5.2 hits EOL in two weeks, Ruby 2.6 hit EOL a month ago, but personally I think maintaining EOL Rubies like 2.5 is easier than older Rails versions. devise version 4 was released in 2016.

Since old versions of the gem would continue to work for users of older versions, I'd like to propose the following cleanup:

  • Test Ruby 2.7, 3.0, 3.1 (I'd go as low as 2.5 if necessary)
  • Test Rails 6.0, 6.1, 7.0 (5.2 only if absolutely necessary)
  • Test devise 4.x (dropping support for 3.x)

What do you think?

PS: I haven't taken a closer look at mongoid support and if bundle exec appraisal rake tests it or if I have to do something special. I went with what the CI setup did.

@pglombardo
Copy link

As an outside observer looking for Rails 7 support, that sounds like a good plan @aried3r. 😄

Any update on Rails 7 support? I'd like to upgrade the opensource project I maintain but am blocked by this gem (which is a great and very helpful gem).

Willing to jump in if needed.

@gonzalo-bulnes
Copy link
Owner

Hi everyone! I'm catching up on the thread.

Actually it seems like mongoid is only a dependancy for development [...]
Can this potentially be ignored or why do you need it in development mode?

Mongoid is a "peer dependency". It made little sense to me to require it if you don't use it, but since support for Mongoid was added, I wouldn't remove it without careful consideration (and a major versiom change).

@gonzalo-bulnes
Copy link
Owner

@aried3r You're certainly making a good argument for scoping the supported Rails version range. In my experience, Rails support tends indeed to require more compromises than supporting older Rubies. (example)

I have tried to keep support until now to avoid breaking backwards compatibility. More by principle than for any practical reason, so it may be time to do a pragmatic pruning and make a 2.0.0 release.

I'll get to the test failures in a separate comment.

@gonzalo-bulnes
Copy link
Owner

@aried3r Regarding the test failures, the output is certainly not very helpful! Your hypothesis makes a lot of sense to me. I'm not sure how to confirm that, but it seems like getting a replacement for Travis CI would be a good start. (I've seem your suggestion of GitHub Actions in #391)

@gonzalo-bulnes
Copy link
Owner

Adding to my comment on @aried3r proposal for support pruning: dropping support for devise < 4 seems reasonable to me in a 2.0.0 release. Devise is a security-critical dependency, and as much as I don't think it's helpful to over-constrain requirments, a little nudge to upgrade Devise seems reasonable to me.

@gonzalo-bulnes
Copy link
Owner

gonzalo-bulnes commented Nov 10, 2022

To everyone in the thread, thank you. I know my involvment has been long to come, and I appreciate how you've stepped in to research, discuss, write code for, test and provide feedback on #391. This is exactly what many people starting open-source project wish to see. It is what I've wished to see for sure.

Maintaining whatever project is a significant amount of work in the long run, and I think it's fair to say that my earlier attempts to get this kind of dynamics going were not successful. I'm very glad to see this (especially because it's been somewhat unexpected!), and I want to acknowledge @mmhan @OskarEichler @aried3r @pglombardo and everybody else involved for your work on this. ❤️

In the meantime, Travis CI changed their policies, which is a bit of a set back. I'd love to get some alternative going so that we can release either 1.18.0 or 2.0.0 in good conditions. I'll do my research on GitHub Actions per @aried3r suggestion (I haven't used them yet). In case it's not obvious: any input or code is appreciated, as you might have noticed that I've had a few competing priorities 😐 but I'll do my best to support the collective effort to get the release out!

@pglombardo
Copy link

Travis CI changed their policies, which is a bit of a set back. I'd love to get some alternative going so that we can release either 1.18.0 or 2.0.0 in good conditions.

Feel free to copy .github/workflows/ruby.yml file to enable Github Actions. Also suggested in #391.

@letz
Copy link

letz commented Dec 5, 2022

what's the state of this? Does anyone need help to push this forward? I don't mind to help

@gonzalo-bulnes
Copy link
Owner

Closed via #401

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

No branches or pull requests

9 participants