dry-system icon indicating copy to clipboard operation
dry-system copied to clipboard

Importer should raise errors if new imports are registered after finalize

Open timriley opened this issue 2 years ago • 2 comments

This would also mean we don't need to worry about handling this error from the Hanami side of dry-system usage.

timriley avatar Feb 08 '22 22:02 timriley

Hey, I want to tackle this issue but not sure where to start. According to my experiments a container that has been finalized will raise an error when another dependency is registered:

require "dry/system/container"

class Application < Dry::System::Container
  configure do |config|
    config.root = Pathname("./my/app")
  end
end


require "logger"
Application.register("logger", Logger.new($stdout))
Application.finalize!
Application.register("logger2", Logger.new($stdout))

Application["logger"].info("Hi")
Application["logger2"].info("Hi")

Raises following error:

➜ ruby dry-system-2.rb
/home/benjamin/.gem/ruby/3.1.2/gems/dry-container-0.9.0/lib/dry/container/registry.rb:41:in `[]=': can't modify frozen Concurrent::Hash: {"logger"=>#<Dry::Container::Item::Callable:0x00007fd1caba8828 @item=#<Logger:0x00007fd1caba9020 @level=0, @progname=nil, @default_formatter=#<Logger::Formatter:0x00007fd1caba8ee0 @datetime_format=nil>, @formatter=nil, @logdev=#<Logger::LogDevice:0x00007fd1caba8d00 @shift_period_suffix=nil, @shift_size=nil, @shift_age=nil, @filename=nil, @dev=#<IO:<STDOUT>>, @binmode=false, @mon_data=#<Monitor:0x00007fd1caba8c38>, @mon_data_owner_object_id=60>>, @options={:call=>false}>} (FrozenError)
        from /home/benjamin/.gem/ruby/3.1.2/gems/dry-container-0.9.0/lib/dry/container/registry.rb:41:in `block in call'
        from /home/benjamin/.gem/ruby/3.1.2/gems/dry-container-0.9.0/lib/dry/container/registry.rb:36:in `synchronize'
        from /home/benjamin/.gem/ruby/3.1.2/gems/dry-container-0.9.0/lib/dry/container/registry.rb:36:in `call'
        from /home/benjamin/.gem/ruby/3.1.2/gems/dry-container-0.9.0/lib/dry/container/mixin.rb:105:in `register'
        from dry-system-2.rb:16:in `<main>'

I probably miss something obvious here, as I don't really know the concept of a Importer and can't find it in the docs. The only related part mentioning Import is:

# system/import.rb
require "system/container"
Import = Application.injector

# In a class definition you simply specify what it needs
# lib/post_publisher.rb
require "import"
class PostPublisher
  include Import["logger"]

  def call(post)
    # some stuff
    logger.debug("post published: #{post}")
  end
end

Which to my knowledge just uses the previously defined container and leverages auto-injection to inject the logger as dependency.

tak1n avatar May 04 '22 18:05 tak1n

Nvm, I think I found the proper thing in the code https://github.com/dry-rb/dry-system/blob/6f98b2174ae33850ff99b79a02c525ac2d779663/lib/dry/system/importer.rb#L16

tak1n avatar May 05 '22 07:05 tak1n

Fixed in #254

timriley avatar Oct 26 '22 11:10 timriley