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

Deprecation warning in Rails 6.0 #134

Open
marckohlbrugge opened this issue Jun 17, 2020 · 10 comments
Open

Deprecation warning in Rails 6.0 #134

marckohlbrugge opened this issue Jun 17, 2020 · 10 comments

Comments

@marckohlbrugge
Copy link

The README has the following example:

StripeEvent.configure do |events|
  events.all BillingEventLogger.new(Rails.logger)
  events.subscribe 'customer.created', CustomerCreated.new
end

This leads to a deprecation warning in Rails 6.0 and will stop working in a future version of Rails

DEPRECATION WARNING: Initialization autoloaded the constants BillingEventLogger and CustomerCreated.

Being able to do this is deprecated. Autoloading during initialization is going
to be an error condition in future versions of Rails.

Reloading does not reboot the application, and therefore code executed during
initialization does not run again. So, if you reload CustomerCreated, for example,
the expected changes won't be reflected in that stale Module object.

These autoloaded constants have been unloaded.

Please, check the "Autoloading and Reloading Constants" guide for solutions.

It would be nice if we could do something like this instead:

StripeEvent.configure do |events|
  events.all 'BillingEventLogger', Rails.logger
  events.subscribe 'customer.created', 'CustomerCreated'
end

I'm not sure about passing Rails.logger that way, but you get the idea. We're passing a string instead which StripeEvent later constantizes.

What do you think?

@rmm5t
Copy link
Member

rmm5t commented Jun 21, 2020

I'm not sure I'm following the need to modify stripe_event. What's the harm in adding some explicit require statements for BillingEventLogger and CustomerCreated?

@marckohlbrugge
Copy link
Author

@rmm5t It breaks code reloading in development.

@marckohlbrugge
Copy link
Author

I also can't get things to work with explicit require statements, although that might be due to my limited understanding of how Rails deals with them. As I'm used to Rails taking care of auto loading classes for me.

Have you been able to use stripe_event with Zeitwerk and Subscriber objects? It might be worth updating the README with sample code that works with Zeitwerk as that seems to be the new standard going forward.

@ikataitsev
Copy link

Hey @marckohlbrugge, you can make it work by using this: https://edgeguides.rubyonrails.org/autoloading_and_reloading_constants.html#autoloading-when-the-application-boots.

@AxelTheGerman
Copy link

Hey @marckohlbrugge, you can make it work by using this: https://edgeguides.rubyonrails.org/autoloading_and_reloading_constants.html#autoloading-when-the-application-boots.

Awesome this works, simply wrap your code in the initializer as shown in the Rails guide (which doesn't give too much detail)

Rails.application.reloader.to_prepare do
  StripeEvent.configure do |events|
    events.all BillingEventLogger.new(Rails.logger)
    events.subscribe 'customer.created', CustomerCreated.new
  end
end

Wonder id this would work or break in older Rails versions and if it should be A) added to the example in the Readme or B) add an extra note for Rails 6+

@jaredbeck
Copy link

The guide says:

For historical reasons, this callback [to_prepare] may run twice. The code it executes must be idempotent.

Is events.subscribe idempotent? Is it OK to "subscribe" twice to the same event?

@RSO
Copy link

RSO commented Sep 1, 2021

The guide says:

For historical reasons, this callback [to_prepare] may run twice. The code it executes must be idempotent.

Is events.subscribe idempotent? Is it OK to "subscribe" twice to the same event?

I just tested this out locally, and no, events.subscribe is not idempotent and instead registers the same handler again. So anytime that I make a change to my Service object the handler gets re-registered, which is not ideal for debugging purposes. Not sure how to solve this, it'd be great if there could be a proper autoloading and auto-reloading system for StripeEvent.

@exocode
Copy link

exocode commented Apr 21, 2022

Are there news on that issue?

WARN: DEPRECATION WARNING:
Initialization autoloaded the constants
StripeEvents, StripeEvents::BillingEventLogger,
StripeEvents::CustomerCreated

I am doing something like this:

StripeEvent.configure do |events|
  events.all StripeEvents::BillingEventLogger.new(Rails.logger)
  events.subscribe 'customer.created', StripeEvents::CustomerCreated.new
end

and in my /app/models/stripe_events/customer_created.rb

I've this:

module StripeEvents
  class CustomerCreated
    def call(event)
      puts "👍🏻 Customer #{event.data.object.id} has been created"
      # Event handling
    end
  end
end

@tannerhallman
Copy link

tannerhallman commented Mar 14, 2023

For us (using docker), this is still very much an issue in local development. Either we have to

  1. Choose that a new class is added for each reload of the code when using this approach leading to multiple processing of the same stripe event:
# /initializers/stripe.rb

Rails.application.reloader.to_prepare do
  StripeEvent.configure do |events|
    events.all BillingEventLogger.new(Rails.logger)
    events.subscribe 'customer.created', CustomerCreated.new
  end
end
  1. Or we have to rebuild our container for each code change.

What's worse? Having a way to replace the instantiated event classes instead of appending a new instance during the reloads to support code reloading is critical for us.

@AxelTheGerman
Copy link

Long time since I looked at this issue but just dug a little deeper...

  1. subscribe simply forwards the call to the backend

backend.subscribe namespace.to_regexp(name), adapter.call(callable)

  1. the backend is ActiveSupport::Notification

self.backend = ActiveSupport::Notifications

  1. ActiveSupport::Notification allows us to unsubscribe either by passing a specific subscriber instance (which will be tricky in this case) OR passing the name of the subscriber, e.g. ActiveSupport::Notifications.unsubscribe('render_template.action_view') - see https://api.rubyonrails.org/classes/ActiveSupport/Notifications.html#module-ActiveSupport::Notifications-label-Manual+Unsubscription

So what we could do:

  1. WORKAROUND: manually unsubscribe via StripeEvent.backend.unsubscribe StripeEvent.namespace.to_regexp('customer.created')

  2. Add an unsubscribe method to StripeEvent (would be similar to above but wouldn't require knowledge about the namespace part).

    def unsubscribe_all(name)
      backend.unsubscribe namespace.to_regexp(name)
    end

I don't think adding unsubscribe for specific instances makes much sense since callable in subscribe is wrapped in adapter.call and we probably don't keep track of the actual object outside of StripeEvent but maybe there are ways and use cases.

  1. Add a flag to subscribe, e.g.clear: false which would allow to optionally remove previous subscriptions

Subscribe now:

    def subscribe(name, callable = nil, &block)
      callable ||= block
      backend.subscribe namespace.to_regexp(name), adapter.call(callable)
    end

With clear:

    def subscribe(name, callable = nil, clear: false, &block)
      callable ||= block
      notification = namespace.to_regexp(name)
      backend.unsubscribe(notification) if clear
      backend.subscribe notification, adapter.call(callable)
    end

I don't have the time right now to open a PR and I haven't noticed any double processing or similar issues in my deployment, but if someone would like to take a stab at it, I'm happy to help

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

8 participants