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

Add support for Rails::Engine #49

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zlw
Copy link
Contributor

@zlw zlw commented Sep 19, 2021

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 :bowtie:

  • There's no way for an Engine to define its own Container
  • There's no way to not have Container for main app (sounds odd but that's actually our use case, we want to have DRY-rb setup only in one Engine)

I think each Engine should define its own Container, otherwise we're loosing whole namespace isolation that Engines provide

MainApp::Container.resolve('users.create')        #> Dry::Container::Error: Nothing registered with the key "users.create"
SuperComponent::Container.resolve('users.create') #> #<SomeComponent::Users::Create:0x00007fa6d963e368>

@zlw zlw force-pushed the zlw/engine-support-latest-dry-system branch 3 times, most recently from 7773474 to 7912c95 Compare September 19, 2021 20:56
@zlw zlw marked this pull request as ready for review September 19, 2021 20:59
@zlw zlw requested a review from solnic as a code owner September 19, 2021 20:59
#
# @api public
def self.container(name, &block)
_container_blocks[name] << block
Copy link
Contributor Author

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... 😅

Copy link
Member

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?)

@solnic
Copy link
Member

solnic commented Dec 23, 2021

Please rebase this against the latest master as your other PR got just merged 🙂

@zlw zlw force-pushed the zlw/engine-support-latest-dry-system branch from 7912c95 to 2a0259b Compare December 23, 2021 16:26
@zlw zlw force-pushed the zlw/engine-support-latest-dry-system branch from d318b9a to a8e5c8b Compare December 26, 2021 16:44
@zlw zlw force-pushed the zlw/engine-support-latest-dry-system branch from a8e5c8b to 8df8ff5 Compare December 26, 2021 16:50
@zlw
Copy link
Contributor Author

zlw commented Dec 26, 2021

@solnic done 👌 it should be ready to go now 🙌

@solnic
Copy link
Member

solnic commented Dec 27, 2021

@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.

@zlw zlw force-pushed the zlw/engine-support-latest-dry-system branch from 8760f39 to 35d2a5a Compare December 27, 2021 21:45
@zlw
Copy link
Contributor Author

zlw commented Dec 27, 2021

@solnic no worries, if something arises from this - I'll do my best to fix it 👌 😄

@zlw zlw force-pushed the zlw/engine-support-latest-dry-system branch from d64d1ea to d6a70e9 Compare December 28, 2021 14:56
@zlw zlw force-pushed the zlw/engine-support-latest-dry-system branch from d6a70e9 to 480d19f Compare December 28, 2021 15:01
@zlw zlw requested a review from solnic December 29, 2021 13:35
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.

2 participants