Skip to content

Commit 3a324fb

Browse files
avolkoviavolkovi
committed
PR feedback
1 parent d13b99c commit 3a324fb

21 files changed

+260
-254
lines changed

‎packages-exp/auth-exp/index.webworker.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ export function getAuth(app = getApp()): Auth {
4646
// background, meanwhile the auth object may be used by the app.
4747
// eslint-disable-next-line @typescript-eslint/no-floating-promises
4848
_getInstance<Persistence>(indexedDBLocalPersistence)
49-
.isAvailable()
49+
._isAvailable()
5050
.then(avail => {
5151
const deps = avail ? { persistence: indexedDBLocalPersistence } : {};
5252
_initializeAuthInstance(auth, deps);

‎packages-exp/auth-exp/src/core/auth/auth_impl.test.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ describe('core/auth/auth_impl', () => {
8383
return testUser(auth, `${n}`);
8484
});
8585

86-
persistenceStub.set.callsFake(() => {
86+
persistenceStub._set.callsFake(() => {
8787
return new Promise(resolve => {
8888
// Force into the async flow to make this test actually meaningful
8989
setTimeout(() => resolve(), 1);
@@ -92,7 +92,7 @@ describe('core/auth/auth_impl', () => {
9292

9393
await Promise.all(users.map(u => auth.updateCurrentUser(u)));
9494
for (let i = 0; i < 10; i++) {
95-
expect(persistenceStub.set.getCall(i)).to.have.been.calledWith(
95+
expect(persistenceStub._set.getCall(i)).to.have.been.calledWith(
9696
sinon.match.any,
9797
users[i].toJSON()
9898
);
@@ -101,7 +101,7 @@ describe('core/auth/auth_impl', () => {
101101

102102
it('setting to null triggers a remove call', async () => {
103103
await auth.updateCurrentUser(null);
104-
expect(persistenceStub.remove).to.have.been.called;
104+
expect(persistenceStub._remove).to.have.been.called;
105105
});
106106

107107
it('should throw an error if the user is from a different tenant', async () => {
@@ -118,7 +118,7 @@ describe('core/auth/auth_impl', () => {
118118
it('sets currentUser to null, calls remove', async () => {
119119
await auth.updateCurrentUser(testUser(auth, 'test'));
120120
await auth.signOut();
121-
expect(persistenceStub.remove).to.have.been.called;
121+
expect(persistenceStub._remove).to.have.been.called;
122122
expect(auth.currentUser).to.be.null;
123123
});
124124
});
@@ -295,7 +295,7 @@ describe('core/auth/auth_impl', () => {
295295

296296
beforeEach(() => {
297297
user = testUser(auth, 'uid');
298-
persistenceStub.get.returns(Promise.resolve(user.toJSON()));
298+
persistenceStub._get.returns(Promise.resolve(user.toJSON()));
299299
});
300300

301301
it('should update the current user', async () => {
@@ -320,7 +320,7 @@ describe('core/auth/auth_impl', () => {
320320

321321
context('now logged out', () => {
322322
beforeEach(() => {
323-
persistenceStub.get.returns(Promise.resolve(null));
323+
persistenceStub._get.returns(Promise.resolve(null));
324324
});
325325

326326
it('should log out', async () => {
@@ -334,7 +334,7 @@ describe('core/auth/auth_impl', () => {
334334

335335
context('still logged in as same user', () => {
336336
it('should do nothing if nothing changed', async () => {
337-
persistenceStub.get.returns(Promise.resolve(user.toJSON()));
337+
persistenceStub._get.returns(Promise.resolve(user.toJSON()));
338338

339339
await auth._onStorageEvent();
340340

@@ -346,7 +346,7 @@ describe('core/auth/auth_impl', () => {
346346
it('should update fields if they have changed', async () => {
347347
const userObj = user.toJSON();
348348
userObj['displayName'] = 'other-name';
349-
persistenceStub.get.returns(Promise.resolve(userObj));
349+
persistenceStub._get.returns(Promise.resolve(userObj));
350350

351351
await auth._onStorageEvent();
352352

@@ -360,7 +360,7 @@ describe('core/auth/auth_impl', () => {
360360
const userObj = user.toJSON();
361361
(userObj['stsTokenManager'] as any)['accessToken'] =
362362
'new-access-token';
363-
persistenceStub.get.returns(Promise.resolve(userObj));
363+
persistenceStub._get.returns(Promise.resolve(userObj));
364364

365365
await auth._onStorageEvent();
366366

@@ -376,7 +376,7 @@ describe('core/auth/auth_impl', () => {
376376
context('now logged in as different user', () => {
377377
it('should re-login as the new user', async () => {
378378
const newUser = testUser(auth, 'other-uid', undefined, true);
379-
persistenceStub.get.returns(Promise.resolve(newUser.toJSON()));
379+
persistenceStub._get.returns(Promise.resolve(newUser.toJSON()));
380380

381381
await auth._onStorageEvent();
382382

‎packages-exp/auth-exp/src/core/auth/auth_impl.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ export class AuthImplCompat<T extends User> implements Auth, _FirebaseService {
111111
* If the persistence is changed in another window, the user manager will let us know
112112
*/
113113
async _onStorageEvent(): Promise<void> {
114-
const user = await this.persistenceManager!.getCurrentUser();
114+
const user = await this.assertedPersistence.getCurrentUser();
115115

116116
if (!this.currentUser && !user) {
117117
// No change, do nothing (was signed out and remained signed out).
@@ -130,7 +130,6 @@ export class AuthImplCompat<T extends User> implements Auth, _FirebaseService {
130130

131131
// Update current Auth state. Either a new login or logout.
132132
await this.updateCurrentUser(user);
133-
// TODO: If a new user is signed in, enabled popup and redirect on that user.
134133
// Notify external Auth changes of Auth change event.
135134
this.notifyAuthListeners();
136135
}

‎packages-exp/auth-exp/src/core/auth/firebase_internal.test.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,11 @@ describe('src/core/auth/firebase_internal', () => {
7070
beforeEach(async () => {
7171
user = testUser(auth, 'uid', undefined, true);
7272
await auth.updateCurrentUser(user);
73-
sinon
74-
.stub(user.stsTokenManager, 'getToken')
75-
.returns(Promise.resolve('new-access-token'));
73+
let i = 0;
74+
sinon.stub(user.stsTokenManager, 'getToken').callsFake(async () => {
75+
i += 1;
76+
return `new-access-token-${i}`;
77+
});
7678
sinon
7779
.stub(user, '_startProactiveRefresh')
7880
.callsFake(() => (isProactiveRefresh = true));

‎packages-exp/auth-exp/src/core/persistence/in_memory.test.ts

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -28,28 +28,28 @@ describe('core/persistence/in_memory', () => {
2828
it('should work with persistence type', async () => {
2929
const key = 'my-super-special-persistence-type';
3030
const value = PersistenceType.LOCAL;
31-
expect(await persistence.get(key)).to.be.null;
32-
await persistence.set(key, value);
33-
expect(await persistence.get(key)).to.be.eq(value);
34-
expect(await persistence.get('other-key')).to.be.null;
35-
await persistence.remove(key);
36-
expect(await persistence.get(key)).to.be.null;
31+
expect(await persistence._get(key)).to.be.null;
32+
await persistence._set(key, value);
33+
expect(await persistence._get(key)).to.be.eq(value);
34+
expect(await persistence._get('other-key')).to.be.null;
35+
await persistence._remove(key);
36+
expect(await persistence._get(key)).to.be.null;
3737
});
3838

3939
it('should work with user', async () => {
4040
const key = 'my-super-special-user';
4141
const auth = await testAuth();
4242
const value = testUser(auth, 'uid');
4343

44-
expect(await persistence.get(key)).to.be.null;
45-
await persistence.set(key, value.toJSON());
46-
expect(await persistence.get(key)).to.eql(value.toJSON());
47-
expect(await persistence.get('other-key')).to.be.null;
48-
await persistence.remove(key);
49-
expect(await persistence.get(key)).to.be.null;
44+
expect(await persistence._get(key)).to.be.null;
45+
await persistence._set(key, value.toJSON());
46+
expect(await persistence._get(key)).to.eql(value.toJSON());
47+
expect(await persistence._get('other-key')).to.be.null;
48+
await persistence._remove(key);
49+
expect(await persistence._get(key)).to.be.null;
5050
});
5151

5252
it('isAvailable returns true', async () => {
53-
expect(await persistence.isAvailable()).to.be.true;
53+
expect(await persistence._isAvailable()).to.be.true;
5454
});
5555
});

‎packages-exp/auth-exp/src/core/persistence/in_memory.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,29 +29,29 @@ export class InMemoryPersistence implements Persistence {
2929
readonly type = PersistenceType.NONE;
3030
storage: Record<string, PersistenceValue> = {};
3131

32-
async isAvailable(): Promise<boolean> {
32+
async _isAvailable(): Promise<boolean> {
3333
return true;
3434
}
3535

36-
async set(key: string, value: PersistenceValue): Promise<void> {
36+
async _set(key: string, value: PersistenceValue): Promise<void> {
3737
this.storage[key] = value;
3838
}
3939

40-
async get<T extends PersistenceValue>(key: string): Promise<T | null> {
40+
async _get<T extends PersistenceValue>(key: string): Promise<T | null> {
4141
const value = this.storage[key];
4242
return value === undefined ? null : (value as T);
4343
}
4444

45-
async remove(key: string): Promise<void> {
45+
async _remove(key: string): Promise<void> {
4646
delete this.storage[key];
4747
}
4848

49-
addListener(_key: string, _listener: StorageEventListener): void {
49+
_addListener(_key: string, _listener: StorageEventListener): void {
5050
// Listeners are not supported for in-memory storage since it cannot be shared across windows/workers
5151
return;
5252
}
5353

54-
removeListener(_key: string, _listener: StorageEventListener): void {
54+
_removeListener(_key: string, _listener: StorageEventListener): void {
5555
// Listeners are not supported for in-memory storage since it cannot be shared across windows/workers
5656
return;
5757
}

‎packages-exp/auth-exp/src/core/persistence/index.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,10 @@ export interface StorageEventListener {
3737

3838
export interface Persistence {
3939
type: PersistenceType;
40-
isAvailable(): Promise<boolean>;
41-
set(key: string, value: PersistenceValue): Promise<void>;
42-
get<T extends PersistenceValue>(key: string): Promise<T | null>;
43-
remove(key: string): Promise<void>;
44-
addListener(key: string, listener: StorageEventListener): void;
45-
removeListener(key: string, listener: StorageEventListener): void;
40+
_isAvailable(): Promise<boolean>;
41+
_set(key: string, value: PersistenceValue): Promise<void>;
42+
_get<T extends PersistenceValue>(key: string): Promise<T | null>;
43+
_remove(key: string): Promise<void>;
44+
_addListener(key: string, listener: StorageEventListener): void;
45+
_removeListener(key: string, listener: StorageEventListener): void;
4646
}

‎packages-exp/auth-exp/src/core/persistence/persistence_user_manager.test.ts

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,14 @@ function makePersistence(
3737
} {
3838
const persistence: Persistence = {
3939
type,
40-
isAvailable: () => Promise.resolve(true),
41-
set: async () => {},
42-
get() {
40+
_isAvailable: () => Promise.resolve(true),
41+
_set: async () => {},
42+
_get() {
4343
return Promise.resolve(null);
4444
},
45-
remove: async () => {},
46-
addListener(_key: string, _listener: StorageEventListener) {},
47-
removeListener(_key: string, _listener: StorageEventListener) {}
45+
_remove: async () => {},
46+
_addListener(_key: string, _listener: StorageEventListener) {},
47+
_removeListener(_key: string, _listener: StorageEventListener) {}
4848
};
4949

5050
const stub = sinon.stub(persistence);
@@ -70,27 +70,27 @@ describe('core/persistence/persistence_user_manager', () => {
7070
const c = makePersistence();
7171
const search = [a.persistence, b.persistence, c.persistence];
7272
const auth = await testAuth();
73-
b.stub.get.returns(Promise.resolve(testUser(auth, 'uid').toJSON()));
73+
b.stub._get.returns(Promise.resolve(testUser(auth, 'uid').toJSON()));
7474

7575
const out = await PersistenceUserManager.create(auth, search);
7676
expect(out.persistence).to.eq(b.persistence);
77-
expect(a.stub.get).to.have.been.calledOnce;
78-
expect(b.stub.get).to.have.been.calledOnce;
79-
expect(c.stub.get).not.to.have.been.called;
77+
expect(a.stub._get).to.have.been.calledOnce;
78+
expect(b.stub._get).to.have.been.calledOnce;
79+
expect(c.stub._get).not.to.have.been.called;
8080
});
8181

8282
it('uses default user key if none provided', async () => {
8383
const { stub, persistence } = makePersistence();
8484
await PersistenceUserManager.create(auth, [persistence]);
85-
expect(stub.get).to.have.been.calledWith(
85+
expect(stub._get).to.have.been.calledWith(
8686
'firebase:authUser:test-api-key:test-app'
8787
);
8888
});
8989

9090
it('uses user key if provided', async () => {
9191
const { stub, persistence } = makePersistence();
9292
await PersistenceUserManager.create(auth, [persistence], 'redirectUser');
93-
expect(stub.get).to.have.been.calledWith(
93+
expect(stub._get).to.have.been.calledWith(
9494
'firebase:redirectUser:test-api-key:test-app'
9595
);
9696
});
@@ -102,9 +102,9 @@ describe('core/persistence/persistence_user_manager', () => {
102102
const search = [a.persistence, b.persistence, c.persistence];
103103
const out = await PersistenceUserManager.create(auth, search);
104104
expect(out.persistence).to.eq(a.persistence);
105-
expect(a.stub.get).to.have.been.calledOnce;
106-
expect(b.stub.get).to.have.been.calledOnce;
107-
expect(c.stub.get).to.have.been.called;
105+
expect(a.stub._get).to.have.been.calledOnce;
106+
expect(b.stub._get).to.have.been.calledOnce;
107+
expect(c.stub._get).to.have.been.called;
108108
});
109109
});
110110

@@ -121,23 +121,23 @@ describe('core/persistence/persistence_user_manager', () => {
121121
it('#setCurrentUser calls underlying persistence w/ key', async () => {
122122
const user = testUser(auth, 'uid');
123123
await manager.setCurrentUser(user);
124-
expect(persistenceStub.set).to.have.been.calledWith(
124+
expect(persistenceStub._set).to.have.been.calledWith(
125125
'firebase:authUser:test-api-key:test-app',
126126
user.toJSON()
127127
);
128128
});
129129

130130
it('#removeCurrentUser calls underlying persistence', async () => {
131131
await manager.removeCurrentUser();
132-
expect(persistenceStub.remove).to.have.been.calledWith(
132+
expect(persistenceStub._remove).to.have.been.calledWith(
133133
'firebase:authUser:test-api-key:test-app'
134134
);
135135
});
136136

137137
it('#getCurrentUser calls with instantiator', async () => {
138138
const rawObject = {};
139139
const userImplStub = sinon.stub(UserImpl, '_fromJSON');
140-
persistenceStub.get.returns(Promise.resolve(rawObject));
140+
persistenceStub._get.returns(Promise.resolve(rawObject));
141141

142142
await manager.getCurrentUser();
143143
expect(userImplStub).to.have.been.calledWith(auth, rawObject);
@@ -147,7 +147,7 @@ describe('core/persistence/persistence_user_manager', () => {
147147

148148
it('#savePersistenceForRedirect calls through', async () => {
149149
await manager.savePersistenceForRedirect();
150-
expect(persistenceStub.set).to.have.been.calledWith(
150+
expect(persistenceStub._set).to.have.been.calledWith(
151151
'firebase:persistence:test-api-key:test-app',
152152
'SESSION'
153153
);
@@ -171,12 +171,12 @@ describe('core/persistence/persistence_user_manager', () => {
171171
} = makePersistence();
172172
const auth = await testAuth();
173173
const user = testUser(auth, 'uid');
174-
persistenceStub.get.returns(Promise.resolve(user.toJSON()));
174+
persistenceStub._get.returns(Promise.resolve(user.toJSON()));
175175

176176
await manager.setPersistence(nextPersistence);
177-
expect(persistenceStub.get).to.have.been.called;
178-
expect(persistenceStub.remove).to.have.been.called;
179-
expect(nextStub.set).to.have.been.calledWith(
177+
expect(persistenceStub._get).to.have.been.called;
178+
expect(persistenceStub._remove).to.have.been.called;
179+
expect(nextStub._set).to.have.been.calledWith(
180180
'firebase:authUser:test-api-key:test-app',
181181
user.toJSON()
182182
);

0 commit comments

Comments
 (0)