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

Make it possible to patch ActionMailer compiler #1409

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

jeffcarbs
Copy link
Contributor

@jeffcarbs jeffcarbs commented Feb 22, 2023

Motivation

Give a workaround for #1392 where included helper methods are typed by tapioca as if they were mailer methods. As @paracycle points out in that thread, tapioca is technically doing the right thing (see the action_methods implementation) although subjectively it may not always be what the consumer might want.

One suggestion was to use helper Foo instead of include Foo but that may break if Foo is a "concern" and uses included do/end that expected to be included in a ActionMailer::Base since helper Foo includes that module into a vanilla module. For example:

module LogConcern
  extend ActiveSupport::Concern

  included do
    before_action { puts "hey" }
  end

  def log_around(message, &blk)
  end
end

class BaseMailer < ActionMailer::Base
end

class PaymentMailer < BaseMailer
  include LogConcern # Works
end

class PaymentMailer < BaseMailer
  helper LogConcern # undefined method `before_action' for PaymentMailer
end

Implementation

I think there are a few options to address:

  • ❌ [Not doing] The ActionMailer rbi compiler gets more opinionated and explicitly only generates mailer method RBI for methods defined by the mailer itself or any non-abstract parent class (i.e. not any included module ancestors).
    • While I don’t think I’ve ever come across actual mailer methods defined via an included module, it's possible that someone somewhere depends on this functionality.
  • ❌ [Not doing] Expose some configuration to make this something the consumer can configure.
    • There's not really a precedent in tapioca currently so not sure what that would look like. For example, would it be "exclude_method_from_included_moduled: true" or would there be some regex/list of modules/methods to exclude?
  • [This PR] Extract out a helper method in ActionMailer compiler to fetch the action methods so it's easier to patch with some more opinionated logic by the consumer.
    • I think this is the most viable, at least in the short-term

The selected option would make it possible to:

module Tapioca
  module Dsl
    module Compilers
      class ActionMailer
        module ActionMethodsPatch
          def action_methods_for_constant
            # my implementation
          end
        end
        prepend ActionMethodsPatch
      end
    end
  end
end

Happy to change options or open to other suggestions if the maintainers have any thoughts.

Tests

This doesn't change any behavior, but I did backfill a few new tests to lock in the current behavior of the rbi compiler.

@jeffcarbs jeffcarbs requested a review from a team as a code owner February 22, 2023 20:59
@paracycle
Copy link
Member

paracycle commented Feb 23, 2023

Thanks, this makes sense to me.

@paracycle paracycle merged commit d965410 into Shopify:main Feb 23, 2023
@jeffcarbs jeffcarbs deleted the action-mailer branch February 24, 2023 02:59
@paracycle paracycle added the enhancement New feature or request label Mar 9, 2023
@shopify-shipit shopify-shipit bot temporarily deployed to production March 10, 2023 16:06 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants