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

Use a dedicated ActiveSupport::Deprecation #517

Merged
merged 1 commit into from Jun 5, 2023

Conversation

etiennebarrie
Copy link
Contributor

Rails 7.1 will deprecate using the singleton ActiveSupport::Deprecation instance (rails/rails#47354). This defines one for the gem and uses it.

It's also added to the application's set of deprecators, allowing it to be configured along all the other deprecators.

cc @rafaelfranca

Copy link
Member

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

One comment, otherwise looks good to me!

@@ -125,6 +125,10 @@ def configure(&block)
Sprockets.register_postprocessor "application/javascript", ::Sprockets::Rails::SourcemappingUrlProcessor
end

initializer :deprecator do |app|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
initializer :deprecator do |app|
initializer "sprockets-rails.deprecator" do |app|

While the other initializers here don't do this (and probably never will due to backwards incompatibility), I think any new initializers added should be namespaced so that they don't conflict with any other gems.

(Duplicate initializer names has been a problem before: rails/rails#43261 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum there's already a namespace in the sense that all initializers are defined within a Railtie.

$ bin/rails initializers | grep deprecator
Sprockets::Railtie.deprecator

(Rails 7.0 app running with this branch)

It feels like the issue you're mentioning is more a bug/misfeature in Railtie::Initializable but OK.

I see all the other deprecators have a namespace in their initializer's name so that makes sense.

Rails 7.1 will deprecate using the singleton ActiveSupport::Deprecation
instance. This defines one for the gem and uses it.

It's also added to the application's set of deprecators, allowing it to
be configured along all the other deprecators.
@guilleiguaran guilleiguaran merged commit 065cbe8 into rails:master Jun 5, 2023
27 checks passed
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.

None yet

3 participants