Import dry-container

[solnic]

This imports dry-container right into dry-core without moving it under Dry::Core namespace, kind-of like we did with dry-equalizer (this one was moved under Dry::Core but the public interface remained as include Dry::Equalizer(...)).

At the same time, this is now auto-loadable via Zeitwerk, which is a nice bonus 🙂

I checked what happens if you require something that depends on dry-container and it seems to work just fine as long as nothing explicitly requires dry-container. For this to work I still need to tweak dry-container itself so that it can issue a warning if something tries to require it.

[flash-gordon]

Keeping it under Container goes against the convention, we may make it work but it'll still be non-obvious for the user. You see Dry::Container::Mixin, how do you know it comes from dry-core? Ofc, there are ways of doing it (e.g. Module#const_source_location) but it makes it unnecessarily complicated. We only have direct dependency on dry-container in dry-validation, dry-system, dry-types, dry-auto_inject, and dry-effects. Shouldn't we just update them instead?

[flash-gordon]

Shouldn't we just update them instead?

I mean I can do it real quick no problemo :)

[solnic]

Move dry-container to dry-core

[solnic]

Keeping it under Container goes against the convention, we may make it work but it'll still be non-obvious for the user

I'm sorry but I thought we agreed to change this convention and use dry-core as a place for various common APIs that live under Dry namespace, not Dry::Core. I think it's a good idea because from the usage point of view it's just nicer to be able to do include Dry.Equalizer(:foo, :bar) than include Dry::Core.Equalizer(:foo, :bar).

[flash-gordon]

@solnic dry-equalizer doesn't expose any public API beyond a single function, I don't think it's a "fair" comparison FWIW. Besides, dry-equalizer is not really needed in a user codebase. OTOH, I can say the same about dry-container. However, some other APIs dry-core provides are more "public" so I think we should have a discussion each time.

[timriley]

it'll still be non-obvious for the user

I do want to point out that this non-obviousness is compounded by the fact we're doing autoloading now.

It means we're asking users to write require "dry/core" and then be aware that this makes a Dry::Container constant autoloadable. There's nothing they've physically typed out that will give them that hint (same for someone reading that code later). Instead they'll have the rely on our documentation to make that connection clear.

[timriley]

My honest feeling at this point is that the autoloading changes the equation, and that it does make more sense to add this as Dry::Core::Container. Putting it under Core here really does emphasise this that we consider this to be an internal (or at least obscure) facility, and it makes the code loading less surprising, which is important.

This would also prevent clashes with people who're using the original dry-container gem too?

[flash-gordon]

This would also prevent clashes with people who're using the original dry-container gem too?

That's for sure

[solnic]

I don't think that having both Dry::Core::Container and Dry::Container bundled guarantees that things won't clash. Ruby constant lookup is...full of surprises and I'd prefer to deprecate dry-container right away. It can simply do require "dry/core" then see if there's Dry::Core::Container available and if not, warn that you should upgrade to latest dry-core, and if there is, warn that you should ditch dry-container from your Gemfile/gemspec and replace with latest dry-core.

[timriley]

@solnic that's fine, I'm good with your deprecation plans here :)

So does this mean you'll switch it to Dry::Core::Container in this PR? Or are you still thinking it should remain as Dry::Container here?

[solnic]

So does this mean you'll switch it to Dry::Core::Container in this PR?

Yes

[flash-gordon]

I don't think that having both Dry::Core::Container and Dry::Container bundled guarantees that things won't clash. Ruby constant lookup is...full of surprises and I'd prefer to deprecate dry-container right away.

As far as I understand, it'll only be an issue inside dry-core itself where a module refers to Container when Dry::Container is already loaded and Core::Container isn't. Since dry-core doesn't have such references atm it'll be safe to add it. Some hiccups may occur caused by constants referenced from within container, though, but I think it'll be a minor issue, we'll see.