Skip to content

Commit cb1caa0

Browse files
jposunajposuna
authored
Updating the Fireperf SDK to support tree-shaking by removing side-efffects (firebase#3803)
* Updating the Fireperf SDK to support tree-shaking by removing side-effects * Ensuring that init can only be called once. * Exporting trace from index.ts * Ensure we are logging the right information when the required APIs are not available * Updating the definition for onFirstInputDelay to make it more readable * Renaming all variables of the type PerformanceController to 'performanceController' rather than 'performance' * Stop passing the performanceController to the api_service just to immediately pass it back to the callback
1 parent 61ed214 commit cb1caa0

22 files changed

+486
-234
lines changed

‎packages-exp/performance-exp/src/controllers/perf.test.ts

Lines changed: 91 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,12 @@
1818
import { expect } from 'chai';
1919
import { stub } from 'sinon';
2020
import { PerformanceController } from '../controllers/perf';
21-
import { Trace } from '../resources/trace';
2221
import { Api, setupApi } from '../services/api_service';
23-
import { FirebaseApp } from '@firebase/app-types-exp';
2422
import * as initializationService from '../services/initialization_service';
25-
import * as FirebaseUtil from '@firebase/util';
23+
import { SettingsService } from '../services/settings_service';
2624
import { consoleLogger } from '../utils/console_logger';
25+
import { FirebaseApp } from '@firebase/app-types-exp';
26+
import { _FirebaseInstallationsInternal } from '@firebase/installations-types-exp';
2727
import '../../test/setup';
2828

2929
describe('Firebase Performance Test', () => {
@@ -43,95 +43,109 @@ describe('Firebase Performance Test', () => {
4343
options: fakeFirebaseConfig
4444
} as unknown) as FirebaseApp;
4545

46+
const fakeInstallations = ({} as unknown) as _FirebaseInstallationsInternal;
47+
4648
describe('#constructor', () => {
4749
it('does not initialize performance if the required apis are not available', () => {
4850
stub(Api.prototype, 'requiredApisAvailable').returns(false);
49-
stub(initializationService, 'getInitializationPromise');
50-
new PerformanceController(fakeFirebaseApp);
51-
expect(initializationService.getInitializationPromise).not.be.called;
52-
});
53-
it('does not initialize performance if validateIndexedDBOpenable return false', async () => {
54-
stub(Api.prototype, 'requiredApisAvailable').returns(true);
55-
const validateStub = stub(
56-
FirebaseUtil,
57-
'validateIndexedDBOpenable'
58-
).resolves(false);
59-
stub(initializationService, 'getInitializationPromise');
60-
new PerformanceController(fakeFirebaseApp);
61-
await validateStub;
62-
expect(initializationService.getInitializationPromise).not.be.called;
63-
});
64-
65-
it('does not initialize performance if validateIndexedDBOpenable throws an error', async () => {
66-
stub(Api.prototype, 'requiredApisAvailable').returns(true);
67-
const validateStub = stub(
68-
FirebaseUtil,
69-
'validateIndexedDBOpenable'
70-
).rejects();
71-
7251
stub(initializationService, 'getInitializationPromise');
7352
stub(consoleLogger, 'info');
74-
new PerformanceController(fakeFirebaseApp);
75-
try {
76-
await validateStub;
77-
expect(initializationService.getInitializationPromise).not.be.called;
78-
expect(consoleLogger.info).be.called;
79-
} catch (ignored) {}
80-
});
81-
});
53+
const performanceController = new PerformanceController(
54+
fakeFirebaseApp,
55+
fakeInstallations
56+
);
57+
performanceController._init();
8258

83-
describe('#trace', () => {
84-
it('creates a custom trace', () => {
85-
const controller = new PerformanceController(fakeFirebaseApp);
86-
const myTrace = controller.trace('myTrace');
87-
88-
expect(myTrace).to.be.instanceOf(Trace);
89-
});
90-
91-
it('custom trace has the correct name', () => {
92-
const controller = new PerformanceController(fakeFirebaseApp);
93-
const myTrace = controller.trace('myTrace');
94-
95-
expect(myTrace.name).is.equal('myTrace');
96-
});
97-
98-
it('custom trace is not auto', () => {
99-
const controller = new PerformanceController(fakeFirebaseApp);
100-
const myTrace = controller.trace('myTrace');
101-
102-
expect(myTrace.isAuto).is.equal(false);
59+
expect(initializationService.getInitializationPromise).not.be.called;
60+
expect(consoleLogger.info).to.be.calledWithMatch(
61+
/.*Fetch.*Promise.*cookies.*/
62+
);
10363
});
10464
});
10565

106-
describe('#instrumentationEnabled', () => {
107-
it('sets instrumentationEnabled to enabled', async () => {
108-
const controller = new PerformanceController(fakeFirebaseApp);
109-
110-
controller.instrumentationEnabled = true;
111-
expect(controller.instrumentationEnabled).is.equal(true);
66+
describe('#settings', () => {
67+
it('applies the settings if provided', async () => {
68+
const settings = {
69+
instrumentationEnabled: false,
70+
dataCollectionEnabled: false
71+
};
72+
73+
const performance = new PerformanceController(
74+
fakeFirebaseApp,
75+
fakeInstallations
76+
);
77+
performance._init(settings);
78+
79+
expect(performance.instrumentationEnabled).is.equal(false);
80+
expect(performance.dataCollectionEnabled).is.equal(false);
11281
});
11382

114-
it('sets instrumentationEnabled to disabled', async () => {
115-
const controller = new PerformanceController(fakeFirebaseApp);
116-
117-
controller.instrumentationEnabled = false;
118-
expect(controller.instrumentationEnabled).is.equal(false);
83+
it('uses defaults when settings are not provided', async () => {
84+
const expectedInstrumentationEnabled = SettingsService.getInstance()
85+
.instrumentationEnabled;
86+
const expectedDataCollectionEnabled = SettingsService.getInstance()
87+
.dataCollectionEnabled;
88+
89+
const performance = new PerformanceController(
90+
fakeFirebaseApp,
91+
fakeInstallations
92+
);
93+
performance._init();
94+
95+
expect(performance.instrumentationEnabled).is.equal(
96+
expectedInstrumentationEnabled
97+
);
98+
expect(performance.dataCollectionEnabled).is.equal(
99+
expectedDataCollectionEnabled
100+
);
119101
});
120-
});
121-
122-
describe('#dataCollectionEnabled', () => {
123-
it('sets dataCollectionEnabled to enabled', async () => {
124-
const controller = new PerformanceController(fakeFirebaseApp);
125102

126-
controller.dataCollectionEnabled = true;
127-
expect(controller.dataCollectionEnabled).is.equal(true);
103+
describe('#instrumentationEnabled', () => {
104+
it('sets instrumentationEnabled to enabled', async () => {
105+
const performance = new PerformanceController(
106+
fakeFirebaseApp,
107+
fakeInstallations
108+
);
109+
performance._init();
110+
111+
performance.instrumentationEnabled = true;
112+
expect(performance.instrumentationEnabled).is.equal(true);
113+
});
114+
115+
it('sets instrumentationEnabled to disabled', async () => {
116+
const performance = new PerformanceController(
117+
fakeFirebaseApp,
118+
fakeInstallations
119+
);
120+
performance._init();
121+
122+
performance.instrumentationEnabled = false;
123+
expect(performance.instrumentationEnabled).is.equal(false);
124+
});
128125
});
129126

130-
it('sets dataCollectionEnabled to disabled', () => {
131-
const controller = new PerformanceController(fakeFirebaseApp);
132-
133-
controller.dataCollectionEnabled = false;
134-
expect(controller.dataCollectionEnabled).is.equal(false);
127+
describe('#dataCollectionEnabled', () => {
128+
it('sets dataCollectionEnabled to enabled', async () => {
129+
const performance = new PerformanceController(
130+
fakeFirebaseApp,
131+
fakeInstallations
132+
);
133+
performance._init();
134+
135+
performance.dataCollectionEnabled = true;
136+
expect(performance.dataCollectionEnabled).is.equal(true);
137+
});
138+
139+
it('sets dataCollectionEnabled to disabled', () => {
140+
const performance = new PerformanceController(
141+
fakeFirebaseApp,
142+
fakeInstallations
143+
);
144+
performance._init();
145+
146+
performance.dataCollectionEnabled = false;
147+
expect(performance.dataCollectionEnabled).is.equal(false);
148+
});
135149
});
136150
});
137151
});

‎packages-exp/performance-exp/src/controllers/perf.ts

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,40 +14,73 @@
1414
* See the License for the specific language governing permissions and
1515
* limitations under the License.
1616
*/
17-
import { Trace } from '../resources/trace';
17+
1818
import { setupOobResources } from '../services/oob_resources_service';
1919
import { SettingsService } from '../services/settings_service';
2020
import { getInitializationPromise } from '../services/initialization_service';
2121
import { Api } from '../services/api_service';
2222
import { FirebaseApp } from '@firebase/app-types-exp';
23-
import { FirebasePerformance } from '@firebase/performance-types-exp';
24-
import { setupTransportService } from '../services/transport_service';
23+
import { _FirebaseInstallationsInternal } from '@firebase/installations-types-exp';
24+
import {
25+
PerformanceSettings,
26+
FirebasePerformance
27+
} from '@firebase/performance-types-exp';
2528
import { validateIndexedDBOpenable } from '@firebase/util';
29+
import { setupTransportService } from '../services/transport_service';
2630
import { consoleLogger } from '../utils/console_logger';
2731

2832
export class PerformanceController implements FirebasePerformance {
29-
constructor(readonly app: FirebaseApp) {
33+
private initialized: boolean = false;
34+
35+
constructor(
36+
readonly app: FirebaseApp,
37+
readonly installations: _FirebaseInstallationsInternal
38+
) {}
39+
40+
/**
41+
* This method *must* be called internally as part of creating a
42+
* PerformanceController instance.
43+
*
44+
* Currently it's not possible to pass the settings object through the
45+
* constructor using Components, so this method exists to be called with the
46+
* desired settings, to ensure nothing is collected without the user's
47+
* consent.
48+
*/
49+
_init(settings?: PerformanceSettings): void {
50+
if (this.initialized) {
51+
return;
52+
}
53+
54+
if (settings?.dataCollectionEnabled !== undefined) {
55+
this.dataCollectionEnabled = settings.dataCollectionEnabled;
56+
}
57+
if (settings?.instrumentationEnabled !== undefined) {
58+
this.instrumentationEnabled = settings.instrumentationEnabled;
59+
}
60+
3061
if (Api.getInstance().requiredApisAvailable()) {
3162
validateIndexedDBOpenable()
3263
.then(isAvailable => {
3364
if (isAvailable) {
3465
setupTransportService();
35-
getInitializationPromise().then(
36-
setupOobResources,
37-
setupOobResources
66+
getInitializationPromise(this).then(
67+
() => setupOobResources(this),
68+
() => setupOobResources(this)
3869
);
70+
this.initialized = true;
3971
}
4072
})
4173
.catch(error => {
4274
consoleLogger.info(`Environment doesn't support IndexedDB: ${error}`);
4375
});
76+
} else {
77+
consoleLogger.info(
78+
'Firebase Performance cannot start if the browser does not support ' +
79+
'"Fetch" and "Promise", or cookies are disabled.'
80+
);
4481
}
4582
}
4683

47-
trace(name: string): Trace {
48-
return new Trace(name);
49-
}
50-
5184
set instrumentationEnabled(val: boolean) {
5285
SettingsService.getInstance().instrumentationEnabled = val;
5386
}

‎packages-exp/performance-exp/src/index.ts

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,11 @@
1616
*/
1717

1818
import { FirebaseApp } from '@firebase/app-types-exp';
19-
import { FirebasePerformance } from '@firebase/performance-types-exp';
19+
import {
20+
FirebasePerformance,
21+
PerformanceSettings,
22+
PerformanceTrace
23+
} from '@firebase/performance-types-exp';
2024
import { ERROR_FACTORY, ErrorCode } from './utils/errors';
2125
import { setupApi } from './services/api_service';
2226
import { PerformanceController } from './controllers/perf';
@@ -31,17 +35,28 @@ import {
3135
Component,
3236
ComponentType
3337
} from '@firebase/component';
34-
import { SettingsService } from './services/settings_service';
3538
import { name, version } from '../package.json';
39+
import { Trace } from './resources/trace';
3640

3741
const DEFAULT_ENTRY_NAME = '[DEFAULT]';
3842

39-
export function getPerformance(app: FirebaseApp): FirebasePerformance {
43+
export function getPerformance(
44+
app: FirebaseApp,
45+
settings?: PerformanceSettings
46+
): FirebasePerformance {
4047
const provider = _getProvider(app, 'performance-exp');
4148
const perfInstance = provider.getImmediate() as PerformanceController;
49+
perfInstance._init(settings);
4250
return perfInstance;
4351
}
4452

53+
export function trace(
54+
performance: FirebasePerformance,
55+
name: string
56+
): PerformanceTrace {
57+
return new Trace(performance as PerformanceController, name);
58+
}
59+
4560
const factory: InstanceFactory<'performance-exp'> = (
4661
container: ComponentContainer
4762
) => {
@@ -58,9 +73,7 @@ const factory: InstanceFactory<'performance-exp'> = (
5873
throw ERROR_FACTORY.create(ErrorCode.NO_WINDOW);
5974
}
6075
setupApi(window);
61-
SettingsService.getInstance().firebaseAppInstance = app;
62-
SettingsService.getInstance().installationsService = installations;
63-
return new PerformanceController(app);
76+
return new PerformanceController(app, installations);
6477
};
6578

6679
export function registerPerformance(): void {

‎packages-exp/performance-exp/src/resources/network_request.test.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,24 @@ import { expect } from 'chai';
2121
import { Api, setupApi } from '../services/api_service';
2222
import * as perfLogger from '../services/perf_logger';
2323

24+
import { FirebaseApp } from '@firebase/app-types-exp';
25+
import { PerformanceController } from '../controllers/perf';
26+
import { FirebaseInstallations } from '@firebase/installations-types';
2427
import '../../test/setup';
2528

2629
describe('Firebase Performance > network_request', () => {
2730
setupApi(window);
2831

32+
const fakeFirebaseApp = ({
33+
options: {}
34+
} as unknown) as FirebaseApp;
35+
36+
const fakeInstallations = ({} as unknown) as FirebaseInstallations;
37+
const performanceController = new PerformanceController(
38+
fakeFirebaseApp,
39+
fakeInstallations
40+
);
41+
2942
beforeEach(() => {
3043
stub(Api.prototype, 'getTimeOrigin').returns(1528521843799.5032);
3144
stub(perfLogger, 'logNetworkRequest');
@@ -46,14 +59,15 @@ describe('Firebase Performance > network_request', () => {
4659
} as unknown) as PerformanceResourceTiming;
4760

4861
const EXPECTED_NETWORK_REQUEST = {
62+
performanceController,
4963
url: 'http://some.test.website.com',
5064
responsePayloadBytes: 500,
5165
startTimeUs: 1528523489152135,
5266
timeToResponseInitiatedUs: 7611,
5367
timeToResponseCompletedUs: 8200
5468
};
5569

56-
createNetworkRequestEntry(PERFORMANCE_ENTRY);
70+
createNetworkRequestEntry(performanceController, PERFORMANCE_ENTRY);
5771

5872
expect(
5973
(perfLogger.logNetworkRequest as any).calledWith(
@@ -70,7 +84,7 @@ describe('Firebase Performance > network_request', () => {
7084
responseEnd: 1645360.832443
7185
} as unknown) as PerformanceResourceTiming;
7286

73-
createNetworkRequestEntry(PERFORMANCE_ENTRY);
87+
createNetworkRequestEntry(performanceController, PERFORMANCE_ENTRY);
7488

7589
expect(perfLogger.logNetworkRequest).to.not.have.been.called;
7690
});

0 commit comments

Comments
 (0)