Skip to content

Conversation

@vjousse
Copy link
Contributor

@vjousse vjousse commented Jun 14, 2021

I was going through the README and noticed that the proposed connect function doesn't compile out of the box.

Not sure if it's the best way to fix it, but it works. Don't hesitate if there is a better way!

Cheers

Copy link
Collaborator

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this change since it does take the example from not compiling directly, to at least working; however, should this example be a little less "Lucky" focused? Or should we just go all in on the Lucky example? If the latter, then it should use UserToken since that's a cleaner way to handle it. If we take out the Lucky based code, then we should rescue from JWT::Error instead of Avram::RecordNotFoundError

What are your thoughts @vjousse ?

@vjousse
Copy link
Contributor Author

vjousse commented Jun 14, 2021

Well, to be honest, I don't see the point of not going all in on the Lucky example for now. At least, it gives people a real world example to build on.
I would prefer to give an example for each and every "big" crystal framework rather than giving something generic that is usable in none.

What do you think?

@jwoertink
Copy link
Collaborator

That makes sense to me. I've seen several shards do that, and it makes it easier to integrate than to think "ok, that works with raw Crystal, but how about in Lucky/Amber,etc..."

In that case, the example becomes a lot more simple:

def connect
  UserAuthToken.decode_user_id(token.to_s).try do |user_id|
    self.identifier = user_id.to_s
    self.current_user =  UserQuery.find(user_id)
  end
end

@vjousse vjousse requested a review from jwoertink June 14, 2021 19:03
Copy link
Collaborator

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! Yeah, this looks much cleaner for a Lucky example. Thanks for updating this 👍

@jwoertink jwoertink merged commit 0e76502 into cable-cr:master Jun 14, 2021
@fernandes
Copy link
Collaborator

thank you @vjousse ! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants