Memoize current_user in actions #508
Conversation
|
Memoize does not work with a "Nil". Opened an issue on Lucky to support this use case |
| @@ -25,7 +25,8 @@ abstract class BrowserAction < Lucky::Action | |||
| expose current_user | |||
|
|
|||
| # This method tells Authentic how to find the current user | |||
| private def find_current_user(id) : User? | |||
| # The 'memoize' macro makes sure only one query is issued to find the user | |||
| private memoize def find_current_user(id) : User? | |||
There was a problem hiding this comment.
You can just set this type to User. If it's nil, it just won't memoize. I saw you opened the issue already for that, but until we have that in, this could be a small win.
There was a problem hiding this comment.
I'm not sure I understand. The User can be nil so I can't change the return type to not have nil.
There was a problem hiding this comment.
The memoize right now doesn't allow you to specify a union like User? https://github.com/luckyframework/lucky/blob/master/src/lucky/memoizable.cr#L21 so this should throw a compile time error.
There was a problem hiding this comment.
Yeah this does throw an error so I opened the issue to add support. I’m not sure how we’d get a quick win by setting it to User because that also won’t compile. I am quite tired so maybe just being dense haha
There was a problem hiding this comment.
Now that memoize supports taking args and having a nil value, I think this is fine. 😄
paulcsmith
force-pushed
the
pcs/memoize-current_user
branch
from
112372b
to
4011f85
Compare
|
when there is a query cache will this make sense? |
|
@wontruefree It still might be good so that it doesn't log a cache hit, but the performance gain would be very minimal once the query cache is shipped |
|
I am curious because memorization like this will cause more memory usage for minimal performance with query caching. This might be worth revising when query cache is added. |
|
Good idea! We can revisit once query caching is added 👍 |
|
Just dropping some notes here. Currently neither of these updates would compile. Bug filed here luckyframework/lucky#1396 |
Still need to do this in ApiAction too