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

Check if action mailbox config is defined #135

Merged
merged 5 commits into from
Jul 22, 2024
Merged

Conversation

mullermp
Copy link
Contributor

Potentially fixes #133

@mullermp
Copy link
Contributor Author

mullermp commented Jul 22, 2024

@ssunday @bobf Sorry to bother - but do you think this will fix #133? I think it would but I think there's something smelly about this as it is currently. I tried using ActiveSupport.on_load(:action_mailbox) but to no avail. I also think maybe this engine should not be loaded all the time unless action mailbox is required. Should this also live in a railtie? Let me know what you all think.

@mullermp mullermp changed the title Set action mailbox config inside initializer Check if action mailbox config is defined Jul 22, 2024
@ssunday
Copy link
Contributor

ssunday commented Jul 22, 2024

@mullermp No idea. This seems like it makes sense, but I don't know that much about railties/etc.

@ssunday
Copy link
Contributor

ssunday commented Jul 22, 2024

It's a low-key problem with this gem is that it's providing all sorts of functionality even if you don't want it. Like it's pulling in aws gems that you may not need.

@mullermp
Copy link
Contributor Author

We made the decision a few years ago to bundle all the features because it's 1) easier for us to manage (develop/release) and 2) more "all in one" and "works out of the box" like rails is itself.

I think we want to keep this pattern but I agree we can probably not bring in SDK gems as dependencies but rather check if they are loaded and enable those features as such.

Copy link
Contributor

@ssunday ssunday left a comment

Choose a reason for hiding this comment

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

I'll approve but 🤷🏻

lib/aws/rails/action_mailbox/engine.rb Outdated Show resolved Hide resolved
lib/aws/rails/action_mailbox/engine.rb Outdated Show resolved Hide resolved
@mullermp mullermp merged commit a7810fa into main Jul 22, 2024
17 checks passed
@mullermp mullermp deleted the fix-am-config-loading branch July 22, 2024 21:27
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.

V4 - Usage without ActiveStorage and ActiveMailbox enabled
3 participants