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

Keep Rails::ConsoleMethods's module inclusion to IRB but raise deprecation warning #51760

Conversation

st0012
Copy link
Contributor

@st0012 st0012 commented May 7, 2024

Motivation / Background

@carlosantoniodasilva suggested in this tweet that the Rails::ConsoleMethods extension approach should still be supported.

Detail

Due to some users/libs relying on Rails::ConsoleMethod to extend Rails console, we need to keep including it to IRB's internal.

But to prevent also adding app and helper methods to the console, which are already registered to IRB through its API, we need to remove them from the Rails::ConsoleMethods. Additionally, we should raise deprecation warning when users try to use Rails::ConsoleMethods in this way.

This commit:

  • Removes all methods from Rails::ConsoleMethods that are already registered to IRB. So it's now essentially empty.
  • Raises a deprecation warning when modules are included to Rails::ConsoleMethods or methods are added to it.
  • Adds a test to ensure that the deprecation warning is raised and the methods are available in the console.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label May 7, 2024
Due to some users/libs relying on Rails::ConsoleMethod to extend Rails
console, we need to keep including it to IRB's internal.

But to prevent also adding `app` and `helper` methods to the console,
which are already registered to IRB through its API, we need to remove
them from the Rails::ConsoleMethods. Additionally, we should raise
deprecation warning when users try to use Rails::ConsoleMethods in this
way.

This commit:
- Removes all methods from Rails::ConsoleMethods that are already
  registered to IRB. So it's now essentially empty.
- Raises a deprecation warning when modules are included to Rails::ConsoleMethods
  or methods are added to it.
- Adds a test to ensure that the deprecation warning is raised and the
  methods are available in the console.
@st0012 st0012 force-pushed the backward-compatibility-for-rails-console-extension branch from 201b3ac to 484c026 Compare May 7, 2024 17:55
@rafaelfranca rafaelfranca merged commit dcfabdb into rails:main May 7, 2024
3 of 4 checks passed
@rafaelfranca rafaelfranca deleted the backward-compatibility-for-rails-console-extension branch May 7, 2024 18:38
Copy link
Member

Choose a reason for hiding this comment

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

FYI: basecamp/console1984#111

I imagine it's not the only gem that's requiring this file :/

@carlosantoniodasilva
Copy link
Member

@st0012 thanks for the updates here! appreciate it.

While I think that expecting libraries / frameworks like Rails to extend the console via the new IRB API is fine/great, I'm afraid this might be too much to ask for app devs.

We might need to look into a simpler way to add methods to the console that doesn't require the extra plumbing of creating a class, adding a description, registering the command, etc... not sure.

@ghiculescu maybe we bring back those files and make them just require the new one? They could be removed with the deprecated stuff later.

@st0012
Copy link
Contributor Author

st0012 commented May 10, 2024

We might need to look into a simpler way to add methods to the console that doesn't require the extra plumbing of creating a class, adding a description, registering the command, etc... not sure.

Users can already create IRB commands without specifying meta attributes with v1.13.1. But maybe Rails can provide APIs like: Rails::ConsoleCommand.register(:my_cmd, "optional description") { do_somthing) }, and automatically assign it the Rails console category?

@ghiculescu can you investigate what's causing the failure? My understanding is that the original app.rb and helper.rb were only expected to provide ConsoleMethods and its helper methods for the console block. And from the code you link to, I don't see anything reference to the methods that were in ConsoleMethods originally?

@ghiculescu
Copy link
Member

It’s not my gem (I just use it) but I’ll have a look. I tend to agree with @carlosantoniodasilva though. It looks like things that are public API are just being deleted.

@ghiculescu
Copy link
Member

@st0012
Copy link
Contributor Author

st0012 commented May 14, 2024

What I found after testing console1984 gem locally:

  • Adding app.rb back to include methods.rb doesn't resolve the undefined method thread_mattr_accessor issue. The error requires requiring active_support/core_ext/module/attribute_accessors_per_thread to fix, so it seemed like the dependency is on app.rb's require "active_support/all", not the module itself.
  • Even after addressing the above error, we'd still see errors related to test fixture methods, which also happens WITHOUT the IRB change too (e.g. against 549144b as well)

@st0012
Copy link
Contributor Author

st0012 commented May 14, 2024

I'm 100% ok with adding app.rb back just to require methods.rb. However, console1984 should require active_support/all or active_support/core_ext/module/attribute_accessors_per_thread itself because it's a dependency of the gem and was just loaded by accident.

ghiculescu added a commit to ghiculescu/console1984 that referenced this pull request May 14, 2024
@ghiculescu
Copy link
Member

I've opened basecamp/console1984#112 to address the missing require.

I'm 100% ok with adding app.rb back just to require methods.rb.

That would solve my problem in the short term so I am 😍 on this. Over the long term that gem would need to get updated to the new IRB API I guess.

st0012 added a commit to Shopify/rails that referenced this pull request May 15, 2024
Those files were removed in rails#51760, but gems like `console1984` depend
on these files for legacy Rails console command extensions.
So keeping files around is required for backward-compatibility.
@st0012
Copy link
Contributor Author

st0012 commented May 15, 2024

@ghiculescu @carlosantoniodasilva I added the files back in #51839

st0012 added a commit to Shopify/rails that referenced this pull request May 15, 2024
Those files were removed in rails#51760, but gems like `console1984` depend
on these files for legacy Rails console command extensions.
So keeping files around is required for backward-compatibility.
st0012 added a commit to Shopify/rails that referenced this pull request May 18, 2024
Those files were removed in rails#51760, but gems like `console1984` depend
on these files for legacy Rails console command extensions.
So keeping files around is required for backward-compatibility.
rosa pushed a commit to basecamp/console1984 that referenced this pull request May 21, 2024
rosa added a commit to basecamp/console1984 that referenced this pull request May 21, 2024
rosa added a commit to basecamp/console1984 that referenced this pull request May 21, 2024
rosa added a commit to basecamp/console1984 that referenced this pull request May 21, 2024
rosa added a commit to basecamp/console1984 that referenced this pull request May 22, 2024
st0012 added a commit to Shopify/rails that referenced this pull request May 23, 2024
Those files were removed in rails#51760, but gems like `console1984` depend
on these files for legacy Rails console command extensions.
So keeping files around is required for backward-compatibility.
st0012 added a commit to Shopify/rails that referenced this pull request May 23, 2024
Those files were removed in rails#51760, but gems like `console1984` depend
on these files for legacy Rails console command extensions.
So keeping files around is required for backward-compatibility.
zzak pushed a commit to zzak/rails that referenced this pull request Jun 7, 2024
It helps to find the code/gem that uses deprecated API.

Enhances rails#51760

Previously when running rails console:
```
DEPRECATION WARNING: Extending Rails console through `Rails::ConsoleMethods` is deprecated and will be removed in Rails 8.0.
Please directly use IRB's extension API to add new commands or helpers to the console.
For more details, please visit: https://github.com/ruby/irb/blob/master/EXTEND_IRB.md
 (called from include at /home/wojtek/.asdf/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/railties-7.2.0.beta2/lib/rails/console/methods.rb:6)
Loading development environment (Rails 7.2.0.beta2)
```

After:
```
DEPRECATION WARNING: Extending Rails console through `Rails::ConsoleMethods` is deprecated and will be removed in Rails 8.0.
Please directly use IRB's extension API to add new commands or helpers to the console.
For more details, please visit: https://github.com/ruby/irb/blob/master/EXTEND_IRB.md

Called via `MissionControl::Jobs::Console::Helpers`
 (called from include at /home/wojtek/.asdf/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/railties-7.2.0.beta2/lib/rails/console/methods.rb:6)
Loading development environment (Rails 7.2.0.beta2)
```
zzak pushed a commit to zzak/rails that referenced this pull request Jun 7, 2024
It helps to find the code/gem that uses deprecated API.

Enhances rails#51760

Previously when running rails console:
```
DEPRECATION WARNING: Extending Rails console through `Rails::ConsoleMethods` is deprecated and will be removed in Rails 8.0.
Please directly use IRB's extension API to add new commands or helpers to the console.
For more details, please visit: https://github.com/ruby/irb/blob/master/EXTEND_IRB.md
 (called from include at /home/wojtek/.asdf/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/railties-7.2.0.beta2/lib/rails/console/methods.rb:6)
Loading development environment (Rails 7.2.0.beta2)
```

After:
```
DEPRECATION WARNING: Extending Rails console through `Rails::ConsoleMethods` is deprecated and will be removed in Rails 8.0.
Please directly use IRB's extension API to add new commands or helpers to the console.
For more details, please visit: https://github.com/ruby/irb/blob/master/EXTEND_IRB.md

Called via `MissionControl::Jobs::Console::Helpers`
 (called from include at /home/wojtek/.asdf/installs/ruby/3.3.2/lib/ruby/gems/3.3.0/gems/railties-7.2.0.beta2/lib/rails/console/methods.rb:6)
Loading development environment (Rails 7.2.0.beta2)
```
xjunior pushed a commit to xjunior/rails that referenced this pull request Jun 9, 2024
Those files were removed in rails#51760, but gems like `console1984` depend
on these files for legacy Rails console command extensions.
So keeping files around is required for backward-compatibility.
jianbo pushed a commit to jianbo/setter-with-association that referenced this pull request Jul 8, 2024
Those files were removed in rails#51760, but gems like `console1984` depend
on these files for legacy Rails console command extensions.
So keeping files around is required for backward-compatibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants