Add the internal auth interface including proactive refresh #3655
Conversation
💥 No ChangesetLatest commit: 207e25a Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Binary Size ReportAffected SDKsNo changes between base commit (c7ef26f) and head commit (03e95cd). Test Logs
|
| @@ -271,6 +275,20 @@ export class AuthImplCompat<T extends User> implements Auth, _FirebaseService { | |||
| return `${this.config.authDomain}:${this.config.apiKey}:${this.name}`; | |||
| } | |||
|
|
|||
| _startProactiveRefresh(): void { | |||
| this.isProactiveRefreshEnabled = true; | |||
There was a problem hiding this comment.
do we need this on the auth object? or should it just live exclusively on the user?
There was a problem hiding this comment.
Auth needs to know about it because start/stop are tightly coupled with the current user lifecycle.
| ); | ||
| sinon | ||
| .stub(user, '_startProactiveRefresh') | ||
| .callsFake(() => (isProactiveRefresh = true)); |
There was a problem hiding this comment.
can these just be sinon.spy()'s and you check to make sure they were called?
There was a problem hiding this comment.
I like this better since I'm testing as it gets toggled back and forth. I suppose you could do it by counting the number of times start was called and how many stop was called, but that seems more complicated
|
|
||
| export const _AUTH_COMPONENT_NAME = 'auth-exp'; | ||
| export const _AUTH_INTERNAL_COMPONENT_NAME = 'auth-internal-exp'; |
There was a problem hiding this comment.
is this right? or should it be auth-interop?
There was a problem hiding this comment.
@Feiyang1 for guidance. I believe internal is correct since that's what we do now, but maybe we're changing our language?
https://github.com/firebase/firebase-js-sdk/search?q=%27auth-internal%27&unscoped_q=%27auth-internal%27
| export class ProactiveRefresh { | ||
| private isRunning = false; | ||
|
|
||
| // Node timers and browser timers return fundamentally different types. |
There was a problem hiding this comment.
i have a similar issue in my PR, do you think it's worth extracting the setInterval logic into a common util?
There was a problem hiding this comment.
Eh that would mean adding more code that is compiled deadweight just to circumvent the type system.
| } | ||
|
|
||
| private updateProactiveRefresh(): void { | ||
| if (this.internalListeners.size > 0) { |
There was a problem hiding this comment.
Should we actively refresh for external listeners as well?
There was a problem hiding this comment.
We've definitely had feature requests for it. I think it should still be an optional thing you turn on, exposed through a method on user or something
There was a problem hiding this comment.
Are we going to do it, or this is something for the future?
There was a problem hiding this comment.
I can add it to the new API proposal and see what kind of feedback we get
|
|
||
| export const _AUTH_COMPONENT_NAME = 'auth-exp'; | ||
| export const _AUTH_INTERNAL_COMPONENT_NAME = 'auth-internal-exp'; |
|
The registration part for auth-internal looks good to me. |
sam-gc
force-pushed
the
samgho/internal-methods
branch
from
dc168e7 to
8a7c658
Compare
sam-gc
force-pushed
the
samgho/internal-methods
branch
from
8a7c658 to
207e25a
Compare
* Proactive refresh and internal SDK * Fix tests for node * Formatting * Remove unused file * PR feedback * Formatting
No description provided.