ueberauth / guardian

Elixir Authentication
MIT License
3.44k stars 381 forks source link

Show example MyApp.Guardian.Plug in README #395

Closed KronicDeth closed 7 years ago

KronicDeth commented 7 years ago

Unlike MyApp.Guardian, there is no explanation of where MyApp.Guardian.Plug comes from in README.md. I was only able to figure it out by seeing that Guardian.Plug had a __using__ macro and seeing that it accepted a implementation argument. This was a stumbling block for myself and someone I was mentoring, so I've added an example and included the different naming scheme for Phoenix 1.2 vs Phoenix 1.3 I know Guardian really only talks about Plug, but since a lot of developers use it in Phoenix, I thought it was worth tailoring the examples to the differences in the Phoenix versions. If you don't want to include Phoenix-isms in the docs, I can remove the MyAppWeb.Guardian.Plug example.

oskar1233 commented 7 years ago

Few things:

  1. I'm not sure if the Guardian module should go to MyAppWeb or to MyApp. I think it's project dependent and up to the developers.
  2. Look into the lib/guardian.ex. I think you should use the Guardian itself, not Guardian.Plug.
      if Code.ensure_loaded?(Plug) do
        __MODULE__
        |> Module.concat(:Plug)
        |> Module.create(
          quote do
            use Guardian.Plug, unquote(__MODULE__)
          end,
          Macro.Env.location(__ENV__)
        )
      end
KronicDeth commented 7 years ago

For (1), yes because of (2), it could go in either MyApp or MyAppWeb when using use Guardian, otp: :my_app since it will contain both Guardian, which isn't web-specific and Guardian.Plug, which is web-specific. I suppose it depends on whether the non-Guardian.Plug functions are called directly. If they are, then it can be outside of MyAppWeb, maybe that should be explained somewhere to help newer developers.

For (2), :man_facepalming:, my bad for not reading the __using__ for Guardian and jumping straight to Guardian.Plug.

I think then all that's needed is to show

defmodule MyApp.Guardian.Plug do
  use Guardian, otp_app: :my_app
end

in the Plug section and to same that it will also include the MyApp.Guardian functions.

I'll do that change and rebase.

oskar1233 commented 7 years ago

There is nice piece of examples and explanation in Guardian behaviour doc - maybe you can inspire with it.

I totally agree, it is web related, but for example I for my recent project use Web module just for Phoenix-related things`, not overally web-related. That's why I've said it's up to developer.

doomspork commented 7 years ago

@KronicDeth I'm not 100% sure this PR really adds much value to the README. For one not everyone is using Phoenix (I don't) and secondly without more explanation the two almost identical code examples are actually confusing.

@hassox or @scrogson, any thoughts?

KronicDeth commented 7 years ago

@doomspork ok, the PR is confusing, but so is introducing MyApp.Guardian.Plug usage in the README without explaining how it was defined.

Do you agree that the README should somehow explain how MyApp.Guardian.Plug gets defined? That was the original thing that tripped us up, so MyApp.Guardian.Plug didn't work (with MyApp substituted for my mentee's namespace, of course). I want something in the README that explains how MyApp.Guardian.Plug gets defined.

hassox commented 7 years ago

@KronicDeth hey thanks for highlighting a confusing issue. I can see how this would be a bit of a head scratcher without explanation of some kind in the README.

Unfortunately what you've got in this PR for the README is not required and could in fact work against you.

When you use Guardian if Plug is available in your project, a nested module will be created for you with all the right functions so you can use them like MyApp.Guardian.Plug.encode_and_sign(user)

hassox commented 7 years ago

@KronicDeth did you want to update this to reflect that this is generated for you or should we close it out?

KronicDeth commented 7 years ago

I'll close it for now. We haven't revisited it on my mentee's project. We did try <APP>.Guardian.<TAB> in iex and it didn't complete as <APP>.Guardian.Plug, so I can't confirm the automatic nested module name.