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

Allow Rails 7.1 #107

Merged
merged 4 commits into from
Oct 18, 2023
Merged

Allow Rails 7.1 #107

merged 4 commits into from
Oct 18, 2023

Conversation

frederikspang
Copy link
Contributor

@frederikspang frederikspang commented Oct 5, 2023

Fixes #108

@kevinrobayna
Copy link

@Mange do you think we can get this merged and released?

roadie-rails.gemspec Outdated Show resolved Hide resolved
roadie-rails.gemspec Outdated Show resolved Hide resolved
frederikspang and others added 2 commits October 6, 2023 09:31
Co-authored-by: T.J. Schuck <tj@tjschuck.com>
Co-authored-by: T.J. Schuck <tj@tjschuck.com>
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Maybe a bit more conservative?

roadie-rails.gemspec Outdated Show resolved Hide resolved
roadie-rails.gemspec Outdated Show resolved Hide resolved
@Mange
Copy link
Owner

Mange commented Oct 11, 2023

The reason I supply a maximum release number is because of these facts:

  • Rails do not follow semver.
  • We use private APIs inside of Rails that we're not supposed to be using in order to patch in the Automatic inliners. This makes it very likely to break at some point.
  • It also indicates which versions are actually tested.
  • If it ever breaks on some version, since no previous version sets a maximum Bundler will downgrade to an earlier version of Roadie on a bundle update and you won't get the version you expect with potential security fixes applied.[1]

As it is right now, since there's no test app for Rails 7.1, I don't want to bump this.

What I personally want is to remove the automatic inliner, switch over to a better manual inliner, and then bump a major release of this gem. At that point, there's no reason to indicate a maximum version of Rails or to test every single version.

Patches for any direction are welcome, of course! Either adding tests for Rails 7.1, or working on a proposal for a new inliner system where we stop monkey patching Rails' classes.

[1]: A demonstration: You ask for Roadie and Rails 9.1+. This version is now marked as not working with the latest Roadie. So then Bundler will find the latest release that doesn't forbid that version of Rails. Workaround is to specify a minimum version of Roadie as well, which I hope most people do but experience tells me they don't.

@tjschuck
Copy link
Contributor

@Mange Thanks for the response!

The reason I'm advocating for removing the maximum version, though, is specifically because you're no longer actively maintaining this gem. You said you want to "remove the automatic inliner, switch over to a better manual inliner, and then bump a major release of this gem", but you've also said you're not going to do that. (Which is fine! I'm fully on-board with moving on from open source projects, and I thank you for all the work you've put into it already.)

I would fully agree with you about your proposal if this would be a sustainable answer moving forward, but it seems the much more likely to occur thing that would solve the problem is removing the time-bombed maximum version and let end-users be responsible for verifying whether or not it works for them. (Like right now! If this maximum version wasn't specified, I would have just upgraded to Rails 7.1 already without issue. Instead, I'm blocked by an incorrect maximum version restriction that is out of my control without forking the gem.)

@Mange
Copy link
Owner

Mange commented Oct 11, 2023

I would fully agree with you about your proposal if this would be a sustainable answer moving forward, but it seems the much more likely to occur thing that would solve the problem is removing the time-bombed maximum version and let end-users be responsible for verifying whether or not it works for them.

I find this very compelling and I've been close to the limit of just doing that for some time now, but I'm also afraid that doing so would be irresponsible as it's hard to tell what problems this could cause further down the line.

(Like right now! If this maximum version wasn't specified, I would have just upgraded to Rails 7.1 already without issue. Instead, I'm blocked by an incorrect maximum version restriction that is out of my control without forking the gem.)

I truly empathize with this. Fun fact: Roadie started out as a PR fork for this reason, and then turned out into a hard fork after lack of response.

It's a hard situation. Should I stop actively maintaining by essentially unlocking it to "wreak havoc", or is it better to "lock it down" and really show that the gem is moving over into "untested territory".

In case I'm not explaining well, let me do a simile (or maybe just a very bad analogy): Imagine if when Microsoft dropped Windows 7, they just turned off Windows Defender and let any executable run on it just to make sure it didn't ruin some future version of a program from running there.
Yeah, it doesn't map well, but hopefully it explains how there's two sides to "unmaintained" - either we make this explicit and wait for contributions to keep it... well, maintained, or we unlock it and let anything happen.

However, not much has happened in Rails land since like Rails 5 now. That they would do a very large change around emails now is probably unlikely, unless it's to remove emails or change underlying email library. Action Inbox or whatever it was called looked like it could contain a major refactor, but it did not.


Tell you what! I've explained my position here, so I'll let my users decide. If my caution does not appear to be warranted or being excessive, I'll unlock it. If y'all felt swayed by the reasoning, I'll wait for a PR that contains tests for Rails 7.
Not sure how to tally that vote, maybe react with 🚀 for unlocking it and 👀 for waiting for tests. 🤷

I'll check in next week and see if no new comments have been written since then. I'm also going to listen to any comment written here, obviously.

Oh, and if I get any volunteers to help maintain roadie-rails then I'm also okay with letting them decide. As long as I feel that there's someone "at the wheel" so to speak.

@tjschuck
Copy link
Contributor

It's a hard situation. Should I stop actively maintaining by essentially unlocking it to "wreak havoc", or is it better to "lock it down" and really show that the gem is moving over into "untested territory".

I agree it's hard! I've stopped maintaining a few libraries myself and have had the same debate!

For something effectively abandoned from an active development perspective, my opinion is to let it be liberally installed, and users can figure it out from there (whether that's having it work, breaking so much they need to move on to something else, forking, monkey patching, etc.), but with additional docs and flags to that effect.

For example, harvesthq/chosen has a deprecation notice in its description and a prominent header in the README indicating it's no longer under active development. (That's where you could also put something like "only tested up to Rails 7.0, future versions of Rails may or may not work", for example.) GitHub also lets you mark a repo as publicly archived (another example), which is a slightly more extreme status flag, but certainly helps get the point across.

Lastly, full acknowledgment here of the fact that I could be the one to make the changes you suggested above (namely the 7.1 tests), and I'm not doing it either! But for what it's worth, if this issue hadn't already been opened, I probably would have spent any development time on just removing Roadie altogether based on the "not maintained" announcement instead of trying to PR an upgrade for it.

(Double lastly: this is where I again express my gratitude for the work you've already put into Roadie. Though I'm advocating for the opposite approach than yours, it's only because I want to be able to keep seamlessly using this library, and I fully respect whatever decision you make!)

@PikachuEXE
Copy link
Collaborator

I can help maintain if you want (I help maintain stuff I am using/have used, but there is no public page for that list -,-)

Leaving the max version wide open and wait for bug report (really passive maintenance mode) is ok if you are the maintainer
But for me (if I am a maintainer) it shouldn't be too difficult to test per major rails version (and I don't change version constraint per minor version coz I got many rails gems, though I do add tests for minor versions when possible)

@Keulsss
Copy link

Keulsss commented Oct 16, 2023

Hello, Any news G's ?

@Mange
Copy link
Owner

Mange commented Oct 18, 2023

I've added @PikachuEXE as a collaborator on both this repo and on roadie and announced the addition to avoid surprises where possible. I've accepted contributions before from them and are very happy with the help I've been getting over the years. 😄


@PikachuEXE, please reach out to me at me AT mange.dev to get access to Rubygems.org to publish new versions if you want. I can also publish any new tagged releases myself if you prefer to not have that responsibility. :-)

@PikachuEXE
Copy link
Collaborator

Lint failed due to using old Ruby = ignored

@PikachuEXE PikachuEXE merged commit 501abb3 into Mange:master Oct 18, 2023
5 of 6 checks passed
@PikachuEXE
Copy link
Collaborator

Still waiting for release permission setup
Use master branch if you can't wait

@PikachuEXE
Copy link
Collaborator

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.

Rails 7.1 Support?
7 participants