refactor(gotrue): Remove unused _currentUser field and update currentUser getter logic. #1168
Conversation
…User getter logic. Simplified comments and fixed some nullcheck operators
| @@ -997,7 +992,7 @@ class GoTrueClient { | |||
| } | |||
| } else { | |||
| final shouldEmitEvent = _currentSession == null || | |||
There was a problem hiding this comment.
if current session is null, then the second bang operator in the expression will never reach
| @@ -748,8 +745,7 @@ class GoTrueClient { | |||
| options: options); | |||
| final userResponse = UserResponse.fromJson(response); | |||
|
|
|||
| _currentUser = userResponse.user; | |||
| _currentSession = currentSession?.copyWith(user: userResponse.user); | |||
| _currentSession = currentSession!.copyWith(user: userResponse.user); | |||
There was a problem hiding this comment.
if access token is null, then the code will never reach the bang operator, hence it should be safe to use it here
There was a problem hiding this comment.
I wouldn't say so because there is an async request in between, so the current session can be removed during the request.
There was a problem hiding this comment.
Maybe im missing something, but that request cant modify the currentSession variable.
If im not wrong, that call would fail if the session or accessToken is not valid and throw an Exception, but never reach the bang operator.
Anyway, the main point of this PR is too delete a variable that is never actually used, we can leave the null-aware operator
There was a problem hiding this comment.
Yeah the request itself doesn't change the currentSession, but while the request is going a signout or something else could happen that sets the currentSession to null
There was a problem hiding this comment.
Yeah, just in case for that situation lets switch it back to null-aware op.
Pull Request Test Coverage Report for Build 15383057528Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
The way the library currently works, those removal are okay.
But the comment about a non-null User, but null Session on currentUser will never happen, because it's derived from _currentSession. So we could either remove that comment or don't do the changes of this pr and instead set the _currentUser only in signUp if a User exists, but no Session.
| @@ -748,8 +745,7 @@ class GoTrueClient { | |||
| options: options); | |||
| final userResponse = UserResponse.fromJson(response); | |||
|
|
|||
| _currentUser = userResponse.user; | |||
| _currentSession = currentSession?.copyWith(user: userResponse.user); | |||
| _currentSession = currentSession!.copyWith(user: userResponse.user); | |||
There was a problem hiding this comment.
I wouldn't say so because there is an async request in between, so the current session can be removed during the request.
|
I totally agree, the comment is wrong, the one I modified aswell. I would just leave: |
|
I agree, sounds reasonable! |
… kept the currentUser comments simple
|
@Vinzent03 made the required changes, let me know if you find something else. |
What kind of change does this PR introduce?
Refactor
What is the current behavior?
_currentUser variable is just being stored as a copy of _currentSession.user
What is the new behavior?
The currentUser getter is handling the indirection correctly, no need to duplicate values and assign both each time they change.
Additional context
I have simplified a comment (it was a double negation).
I have added some bang operators in cases where null-aware operator where not needed