-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
fix(supabase): get jwt on http call #540
Conversation
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.
This is brilliant!! I attempted to do what the js client library was doing, but I couldn't find a way to do it without breaking change, but here you are to save the day again! I am loving every bit of the code in this PR! Thank you so much for the amazing work!
Thanks for the kind words 😊 |
Yeah this PR is a bit scary to publish without a more robust integration tests, ins't it? I know that gotrue server has a mechanism to allow token refresh using the same access token for a short period of time (I believe it was 10 seconds) to allow simultaneous token refreshes to happen. Either way, I was thinking about creating some additional tests within the supabase_flutter package that launches a local Supabase instance using the Supabase Github actions and tests basic use cases of all the Supabase components like postgrest or storage from a Flutter app. Then we could include a test case where we try to sending bunch of requests using |
Probably testing realtime would be easier too, I think. But yeah maybe not a huge gain overall as you say. |
My guess was indeed the case and I've added a global completer now to combine these token refresh requests. When trying to implement that, I removed all the other completer that were used for the retry logic. All tests are passing and I think everything is fine. The changes of this pr are more significant than expected, though. I think the code of the retry logic is cleaner as well. |
completer.completeError(error, stack); | ||
_onAuthStateChangeController.addError(error, stack); | ||
rethrow; |
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.
Would removing completeError
cause any errors to not be caught that used to be able to be caught before this update?
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.
I think developers can listen to the errors with onStateChanged
, so I'm not too concerned, but thought I would flag it with you!
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.
I think actually the opposite is the case. Previously a completer got created in _saveSession
causing any errors to get ignored. Now these errors dont get catched. Will investigate and might add further tests. These changes are really though...
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.
I've added a new commit to catch any errors from _callRefreshToken
in _saveSesison
and _setTokenRefreshTimer
, because these should already be added to the stream and be handled there. Testing this with a test is very complicated, because all these methods are private and I would have to create a cusomt http client to mock some responses.
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.
Thank you so much for all the hard work!
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.
I wouldn't mind making things public with the internal annotation like you did with another method or property the other day if that makes testing things easier.
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.
I don't think that extra effort is neccessary. I'm very confident the errors are properly catched. _saveSession
and _setTokenRefreshTimer
are the only ones calling _callRefreshToken
, which are not directly called by the user, so I'm pretty sure that's correct now.
Looks great to me. If you are ready to merge it, let's merge it and we can publish new versions. . |
What is the new behavior?
I'm now getting the latest access token when the actual request is done by wrapping the http client with another client, which gets the newest from
gotrue
and sets the headers correctly.Additionally I made
expiresAt
a late variable to avoid reparsing the jwt each time. And I addedexpired
, because I think its a nice addition to easy check the validity of a session. But I'm not sure if some additional padding could be useful. So that a session is maybe 5 seconds before actual expiration, marked as expired.Additional context
That technique is inspired from supabase-js