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

Remove actionmailer dependency #547

Closed
ckhsponge opened this issue Dec 17, 2024 · 6 comments
Closed

Remove actionmailer dependency #547

ckhsponge opened this issue Dec 17, 2024 · 6 comments

Comments

@ckhsponge
Copy link

Thanks for maintaining this!

I'd like the maintainers to consider removing actionmailer as a required dependency. Including actionmailer increases the size of our gem directory from 21 MB to 54 MB which is a large increase for something we don't need. Half of that increase is from nokogiri which is required by activesupport.

For the project I'm working on we are using the Slack notifier in a Sinatra app running on AWS Lambda. We're trying to keep the project light.

Changing it to a development dependency seems to work great for us:
ckhsponge@e3740c0

Let me know if you'd like a PR.

Thanks!

@kmcphillips
Copy link
Collaborator

That works for you because you're not using the mailer.

Also ActiveSupport is used in several places in the application, outside of test. So it can't be avoided reasonably.

Image

@ckhsponge
Copy link
Author

@kmcphillips Sorry, I misspoke. Active Support is not a problem. ActionMailer requires ActionPack which requires Nokogiri.

I don't see other dependencies on ActionPack so removing ActionMailer even with Active Support present appears to be a big space saver.

@kmcphillips
Copy link
Collaborator

Gotcha.

The issue is that the email notifier is built on ActionMailer:

So the tradeoff here is an undeclared dependency, in the case of someone trying to send an email without requiring rails first. I don't think I like that.

I could separate a minimal require version here so this dependency isn't loaded, but your specific complaint is about bundle size. And that wouldn't help.

@ckhsponge
Copy link
Author

I suppose one way to look at my request would be that I desire to use Exception Notifier without the Email Notifier and the baggage it brings.

We use a number of Rails gems like ActiveModel and ActiveJob that don't require all of Rails. AWS has aws-activejob-sqs that we use as well. That previously required all of Rails but that dependency has been removed. This makes a lot of sense here because SQS can trigger a Lambda and not having to pull in all of Rails is a huge advantage.

Exception Notifier would be more versatile with less required dependencies but if most people are emailing perhaps it's not worth it at the moment. There's also reasonable workarounds. Thanks!

@kmcphillips
Copy link
Collaborator

Ok well this is a clear description of the problem. Thank you.

I know that people tend to think of Rails as monolithic if they don't think too hard, I hope I'm not doing that here too. I'm just learning this codebase.

But from what I understand so far the only way to meet your requirement of not putting actionmailer as dependency of the gem is to require it at runtime, and that pushes the requirement of manually including it to all users of the gem that are not requiring it. It also has this nasty side effect of loading it at runtime for apps that think they're not loading it. I don't think that's worth it.

My recommendation is to maintain a fork that comments out those two lines. But how about as I'm working on understanding and refactoring this I'll keep this in my mind. It'd be great to find a way to split it out.

@ckhsponge
Copy link
Author

Well said. I'll maintain a fork. It's probably not worth putting extraordinary effort in for this at the moment.

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

2 participants