Should libraries implement default callbacks that are intended to always be overwritten?

My opinion very much undecided on this Question so all comments welcome.

This is probably best discussed using the concrete example I am considering at the moment in Raxx.
(Raxx has the goal of being a capable toolkit for developing lean web applications in Elixir.)

For simple endpoints applications implement the Raxx.SimpleServer behaviour.
Simple endpoints are when streaming is not required for either request or response, if streaming is required then the Raxx.Server behaviour is implemented

A simple example would look like the following.

defmodule MyServer do
  use Raxx.SimpleServer

  @impl Raxx.SimpleServer
  def handle_request(%{method: :GET, path: []}, _state) do
    response(:ok)
    |> set_header("content-type", "text/plain")
    |> set_body("Hello, World!")
  end
end

The line use Raxx.SimpleServer defined a default implementation of handle_request/2 which any application is always expected to overwrite.
The default implementation shows a page that explains what a user should be implementing, and this is done to help users.

I am no revisiting this choice for the following reasons.

  • behaviours create a compile time warning when one of their callbacks is not implemented,
    this is lost by implementing a default. Any mistakes are found only at runtime, hopefully by tests.
  • Safely generating this page required adding a dependency on eex_html, this dependency is included only to render pages that should never be rendered in a correctly set up application
  • These helper pages are useless if the client is not a human. i.e. when making api services.

In summary I am thinking of removing this default because a compile time warning is more useful than any information that might be show at runtime in the page, and in any case is just a copy of the information available in the documentation

2 Likes

If you want to have a descriptive error page I feel it’s in the same realm as the phoenix error page with the debugging information: Enable it in :dev, but fail for other environments.

2 Likes

Ignoring warnings when compiling dependencies is fine but one should not ignore warnings in their own code – and warnings about non-implemented callbacks are important.

IMO get rid of the help page for exactly the reasons you mentioned.

@LostKobrakai has a point as well; you could keep the current behaviour in :dev environment only.

1 Like

What phoenix does in such cases (like router callbacks into controllers and so forth) is it has a ‘jump’ function (action I think?) that is what actually calls the appropriate method, and if it throws, like it doesn’t exist, then it reports it as such. This means you don’t get compile warnings though as it’s an apply, however with macro work to ‘bake in’ the calls without using apply you could get the best of both worlds? It would require making a pre-call function in that same module and calling that instead, and having it call the real function that needs to be overridden. That would give both a warning, maybe even an error at compile-time, but if such a thing does somehow make it through then it would give a good message.

On the other hand, making a public function with a (mostly) uncallable name that just calls the functions that needs to be implemented locally then it wouldn’t compile if they didn’t exist, but you’d need to craft good default data to let dialyzer handle it properly and that can be pretty hard to do without taking ‘opaque’ arguments in (this function would never actually be called, it’s sole purpose is just to require those functions).

Although at that point you could just analyze the module info(:functions) at compile time from the thing that calls those implementations and verify they exist first, but that would involve requiring those files to be compiled first and the beam files to be scanned.

Honestly, I’d say just implement a behaviour, no default implementations, and let the compiler handle it as normal. :slight_smile:

3 Likes

FWIW, we have been slowly migrating away from default implementations and using optional callbacks instead. If the user wants warnings, then @impl is the way to go. There is an old discussion about it here: Behaviours, defoverridable and implementations

4 Likes

A lot of smart and interesting solutions there. thanks.

But I think I also agree with your summary, let the compiler handle it.

2 Likes