whitfin / cachex

A powerful caching library for Elixir with support for transactions, fallbacks and expirations
https://hexdocs.pm/cachex/
MIT License
1.6k stars 103 forks source link

No longer possible to start Cachex using Supervisor.child_spec/2? #380

Closed HansGlimmerfors closed 1 month ago

HansGlimmerfors commented 1 month ago

It seems that #372 breaks the possibility of starting Cachex with Supervisor.child_spec/2. Previously it was possible to start Cachex using e.g.

defmodule MySupervisor do
  use Supervisor

  def init() do
    Supervisor.init([Supervisor.child_spec({Cachex, [name: :my_cache]}, id: :my_cache_id)], strategy: :one_for_one)
  end
end

If removing the possibility to start Cachex using Supervisor.child_spec/2 is intentional, then maybe Cachex should no longer use Supervisor? Or if Cachex should still support using Supervisor.child_spec/2, then Cachex.start_link/1 should be re-added.

whitfin commented 1 month ago

Ah, that makes sense.

Sorry, I find Supervisor.child_spec/2 to be one of the most confusing things Elixir ever introduced. Nomatter how many times I've read those docs, it still doesn't feel intuitive to me at all 🙃

This is solved by adding something like this to Cachex, right?

def child_spec(options) do
  case Keyword.fetch(options, :name) do
    {:ok, name} ->
      %{
        id: name,
        start: {Cachex, :start_link, [name, options]}
      }

    _invalid ->
      error(:invalid_name)
  end
end

I'm happy to fix this; I just don't want Cachex.start_link/1 to have to exist if at all possible. Relying on a required argument to be stuffed into an options list feels bad. I know the same is true in child_spec/1 but at least that is only in use during supervision.

whitfin commented 1 month ago

Actually, ignore the comment above. I decided to just put back the old Supervisor.start_link/1 signature in https://github.com/whitfin/cachex/commit/7aa2f5d6e9a2b26b8c573ba40b9933b783e06bad. I just added @doc false to dissuade people from calling it directly.

When you get a moment if you could validate your case on latest main, and close this out if it's solved, that'd be great! Thank you for taking the time to file this though, I totally missed it 😔

HansGlimmerfors commented 1 month ago

I can confirm that with the return of Cachex.start_link/1 on main, it's again possible to start Cachex as it was possible in 3.6.0. 😄 A small correction to my initial comment is that it's technically Supervisor.init/2 that is starting Cachex and that's the step that was failing due to the removal of Cachex.start_link/1 - but the use case is closely related to Supervisor.child_spec/2.

This is solved by adding something like this to Cachex, right?

It's also possible to pass a map of values to Supervisor.child_spec/2, in which case the child_spec/1 is never called. To check that the arguments are valid, it would be better to perform the checks in Cachex.start_link/1 instead.

I'll go ahead and close the issue since the problem/incompatibility with previous versions was solved, but I'll try to stick around for a bit of discussion if you choose to comment again on this thread. Thanks for the getting around to the issue so soon! 😃