-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add support for Rails::Engine #49
base: main
Are you sure you want to change the base?
Conversation
7773474
to
7912c95
Compare
# | ||
# @api public | ||
def self.container(name, &block) | ||
_container_blocks[name] << block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit puzzling tbh, when I do: Dry::Rails::Engine.container(...) { do something with container }
it's not available until reload!
call Finalizer
and evaluates the config again 🤔 ("worked like that before")
would the same happen if container is used eg. in before_filter
- changes done to container would "leak" into next request? this doesn't sound right... 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zlw setting up container should only be available during app's booting phase. Once it's done, we should literally freeze its config (which I believe already happens?)
Please rebase this against the latest master as your other PR got just merged 🙂 |
7912c95
to
2a0259b
Compare
d318b9a
to
a8e5c8b
Compare
a8e5c8b
to
8df8ff5
Compare
@solnic done 👌 it should be ready to go now 🙌 |
@zlw thanks, this is something we should definitely support. Personally I don't use engines at the moment, which means I won't be able to help with this other than to provide some basic feedback in this PR + release it. If we get this merged and released and it turns out it breaks things, you'd have to own it and fix things though. Otherwise I would have to revert the entire feature because I wouldn't have time to deal with any potential fixes. |
8760f39
to
35d2a5a
Compare
@solnic no worries, if something arises from this - I'll do my best to fix it 👌 😄 |
d64d1ea
to
d6a70e9
Compare
d6a70e9
to
480d19f
Compare
Hi 👋
Note: This PR builds on top on previous PR where I added support for latest dry-system, but I can't do stacked PRs here 😢 Going through the commits (1st commit - previous PR, 2nd - this PR) might give cleaner diff 🙌
Added experimental support for Rails::Engine
Adding dry-rails to monolith Rails app works like a charm 🥇
However, it's not so great when used with Engines
I think each Engine should define its own Container, otherwise we're loosing whole namespace isolation that Engines provide