-
-
Notifications
You must be signed in to change notification settings - Fork 599
Stateless UserController #846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## master #846 +/- ##
=========================================
+ Coverage 92.08% 92.1% +0.02%
=========================================
Files 54 54
Lines 4939 4953 +14
Branches 1100 1103 +3
=========================================
+ Hits 4548 4562 +14
Misses 391 391
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- "logOut" looks good to me
- "become" will probably make some developers confused. Currently, when become is called, the following operations automatically use the current user session token. That's not true for the Node.js. My suggestion is to leave the become as is now and create a new function (maybe called "me"?).
- Shouldn't we take care of "signUp" and "logIn" as well?
I believe those are already taken care of in a node js environment. |
ugh. I can't quite wrap my head around this -- probably just soft after my holiday! Most of the work I've done with the JS-SDK is cloud code, so I am familiar with the territory, but I could use a use case of what we're trying to accomplish (in the form of an integration test or two) to show what the goal of this change is more clearly. I read #833 twice, but I can't quite piece together what we're solving and why. |
@acinader currently, there is no method in the Node.js SDK to log out or validate a session token. That's the problem @dplewis is solving. This PR does two different things:
|
ok. I get the logout fix and I buy it. Not so sure about the become/me. What use case would you use this become/me for? And where would the session Id be coming from? When I've wanted server code to be able to 'log in', I have made an auth adapter so I can create a session using login. And one other thought that may be wrong: would it make sense to make two user controllers: one for node and one for browser? They could inherit from a common ancestor that has the majority of the functionality, but one would be stateful and the other not. |
Currently the same UserController is used in two different scenarios:
The flow works, except:
I agree with you that we could have two controllers but I'd tackle this on a separate issue. |
whew. Thank you for laying that out so crystal clearly. the only real purpose of me/become in the node environment would be to validate a sessionToken? or is there any other use case? I can't think of one. If that's the only use, I'm good with it. My preference is also On the two controllers, just asking for your view. After reading your response, the thread you cite and playing with some code, I don't think that breaking into an abstract class with two subclasses would really be worth overhead it would take to figure out what is going on. Though I don't have a strong opinion either way :). |
I think it is the only one
In my personal opinion, I think it is easier to understand if we go with the subclasses instead of the current approach in which I have to read the whole code and understand the "if" clauses. I think it is a low priority issue though. |
expect(user.existed()).toBe(true); | ||
}); | ||
|
||
it('can retreive a user with sessionToken and masterKey(me)', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a use case for this? In other words, how would this request differ from the above? The master key is included, but does it behave it any differently? I looked at the router code, but not sure...
Closes: #833
There aren't any CurrentUser in a server environment. This PR allows for use of sessionTokens and prevent errors from being thrown.
Let me know if more tests are needed and any feedback