ueberauth / guardian

Elixir Authentication
MIT License
3.44k stars 381 forks source link

Use a nested module in bitwise to provide Plug callbacks conditionally #404

Closed rbino closed 7 years ago

rbino commented 7 years ago

If it is imported in the scope of the module, the import is executed even if it's inside a false conditional

rbino commented 7 years ago

The alternative for the scoped imports would be using the full function name for halt (that is the only function that is currently used from Plug.Conn) or using an alias and doing something like PC.halt().

Let me know if you prefer this or one of the two alternatives above and I will fix the PR.

rbino commented 7 years ago

If I alias Plug.Conn the compiler warns me

warning: unused alias Conn
  lib/guardian/permissions/bitwise.ex:364

warning: unused alias Pipeline
  lib/guardian/permissions/bitwise.ex:366

(the warning is also already there for GPlug.Pipeline).

If the warning it's not a problem I will proceed by using an alias.

Another possible solution I came up with could be moving the Plug related functions in a separate (possibly nested) module, so that they can be entirely wrapped in a conditional (like all the other Plug dependent modules) and do a conditional defdelegate from the top module, e.g.

  if Code.ensure_loaded?(Plug) do                                                
    defdelegate init(opts), to: Guardian.Permissions.Bitwise.PlugImpl            
    defdelegate call(conn, opts), to: Guardian.Permissions.Bitwise.PlugImpl      

    defmodule PlugImpl do                                                        
      @moduledoc false                                                           

      import Plug.Conn                                                           
      alias Guardian.Permissions.Bitwise, as: GBits                              
      alias Guardian.Plug, as: GPlug                                             
      alias GPlug.Pipeline                                                       

      @doc false                                                                 
      @spec init([GBits.plug_option]) :: [GBits.plug_option]                     
      def init(opts) do                                                          
        ensure = Keyword.get(opts, :ensure)                                      
        one_of = Keyword.get(opts, :one_of)
       ...         

Let me know which of the two approaches you prefer and I will update the PR.

hassox commented 7 years ago

To be honest I'd rather not have a separate plug if we can.

rbino commented 7 years ago

@hassox would the nested module + defimpl be an OK solution (it would not change the current API)?

hassox commented 7 years ago

Actually yeah that looks cool.

rbino commented 7 years ago

@hassox I've changed the PR to the nested module implementation, let me know if it's ok

doomspork commented 7 years ago

This looks good to me @rbino thank you for doing this! @hassox any final thoughts? It would be good to get this in.

scrogson commented 7 years ago

❤️