search

ueberauth / guardian

Elixir Authentication
MIT License
3.43k stars 382 forks source link

V2 #564

Closed yordis closed 5 years ago

yordis commented 5 years ago

Breaking Changes

doomspork commented 5 years ago

I'm definitely onboard with pulling out Phoenix because it includes far more than needed for Guardian to function. I'm not entirely sure the same can be said for Plug.

So my first question is by pulling out Plug what we do hope to gain?

My concern remains: if 99% of Guardian users are using it with Plug in some fashion (including Phoenix) pulling it out of guardian just makes using the library involve that many additional steps. I'm also not convinced Plug so large a dependency that even if you weren't using Guardian for that, you're not pulling in dozens of sub-dependencies resulting in some insane dependency tree.

Also let's keep in mind this just makes even more packages to manage. That's not our strong suit right now 😁

@hassox / @scrogson any thoughts on pulling out Plug vs keeping it?

hassox commented 5 years ago

I'm down to pull out Phoenix but I think pulling out plug will decrease the usefulness of the lib and increase maintenance. Not in favour of extracting plug

doomspork commented 5 years ago

Thank you @hassox πŸ‘Œ

@yordis we can focus on v2 without the inclusion of Phoenix. There may be opportunities to refactor some of the code we hoped to target by pulling Plug out. It would be a good start to document them in issues and start a conversation around them. Collectively we can probably come up with a good solution that provides clear boundaries while keeping the library usage for the vast majority of the users.

Hanspagh commented 5 years ago

Seems like pulling out phoenix is reasonable, but I still think there is some value in the helper functions in socket. Would it somehow make sense to keep these to avoid having people using the sockets, implement this them self? I am not sure how this would be achieved though :confused:

doomspork commented 5 years ago

@Hanspagh I'm not familiar with using sockets outside of Phoenix, when/why would you need that?

codecov-io commented 5 years ago

Codecov Report

Merging #564 into master will decrease coverage by 0.53%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #564      +/-   ##
==========================================
- Coverage   85.51%   84.97%   -0.54%     
==========================================
  Files          18       17       -1     
  Lines         421      406      -15     
==========================================
- Hits          360      345      -15     
  Misses         61       61
Impacted Files Coverage Ξ”
lib/guardian/token/jwt/verify.ex 87.5% <0%> (-12.5%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 65eb6d3...60b8c3e. Read the comment docs.

Hanspagh commented 5 years ago

@doomspork I was still thinking sockets within phoenix, but my thought was that everybody would need the helper functions when working with the phoenix sockets, so it would be nice to have them somewhere, maybe in a guide or a utility library.

doomspork commented 5 years ago

@Hanspagh all of the Phoenix functionality is moving into guardian_phoenix, so folks will still have access to them. We'll update the guides to reflect the new package.

Hanspagh commented 5 years ago

@doomspork I completely missed that, my apologies. The above can just be ignored 😊

doomspork commented 5 years ago

No worries @Hanspagh! Glad we're on the same page 😁