Skip to content

Commit 7d916d9

Browse files
Feiyang1Feiyang1
authored
Move template data under a new customData field (#3946)
* Move template data under a new `customData` field * Create stupid-dots-grab.md * fix tests * fix installations * fix analytics * fix rc * fix auth-exp * fix auth-exp again
1 parent 959e21a commit 7d916d9

File tree

23 files changed

+66
-86
lines changed

23 files changed

+66
-86
lines changed

‎.changeset/stupid-dots-grab.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@firebase/util": patch
3+
---
4+
5+
Write template data to a new `customData` field in` FirebaseError` instead of writing to the error object itself to avoid overwriting existing fields.

‎packages-exp/auth-exp/src/api/index.test.ts

+8-4
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ describe('api/_performApiRequest', () => {
284284
assert.fail('Call should have failed');
285285
} catch (e) {
286286
expect(e.code).to.eq(`auth/${AuthErrorCode.NEED_CONFIRMATION}`);
287-
expect(e._tokenResponse).to.eql({
287+
expect((e as FirebaseError).customData!._tokenResponse).to.eql({
288288
needConfirmation: true,
289289
idToken: 'id-token'
290290
});
@@ -314,7 +314,9 @@ describe('api/_performApiRequest', () => {
314314
assert.fail('Call should have failed');
315315
} catch (e) {
316316
expect(e.code).to.eq(`auth/${AuthErrorCode.CREDENTIAL_ALREADY_IN_USE}`);
317-
expect(e._tokenResponse).to.eql(response);
317+
expect((e as FirebaseError).customData!._tokenResponse).to.eql(
318+
response
319+
);
318320
}
319321
});
320322

@@ -343,8 +345,10 @@ describe('api/_performApiRequest', () => {
343345
assert.fail('Call should have failed');
344346
} catch (e) {
345347
expect(e.code).to.eq(`auth/${AuthErrorCode.EMAIL_EXISTS}`);
346-
expect(e.email).to.eq('[email protected]');
347-
expect(e.phoneNumber).to.eq('+1555-this-is-a-number');
348+
expect((e as FirebaseError).customData!.email).to.eq('[email protected]');
349+
expect((e as FirebaseError).customData!.phoneNumber).to.eq(
350+
'+1555-this-is-a-number'
351+
);
348352
}
349353
});
350354
});

‎packages-exp/auth-exp/src/api/index.ts

+3-1
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,8 @@ function makeTaggedError(
251251
}
252252

253253
const error = AUTH_ERROR_FACTORY.create(code, errorParams);
254-
(error as TaggedWithTokenResponse)._tokenResponse = response;
254+
255+
// We know customData is defined on error because errorParams is defined
256+
(error.customData! as TaggedWithTokenResponse)._tokenResponse = response;
255257
return error;
256258
}

‎packages-exp/auth-exp/src/core/providers/facebook.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ describe('src/core/providers/facebook', () => {
5959
const error = AUTH_ERROR_FACTORY.create(AuthErrorCode.NEED_CONFIRMATION, {
6060
appName: 'foo'
6161
});
62-
(error as TaggedWithTokenResponse)._tokenResponse = {
62+
(error.customData! as TaggedWithTokenResponse)._tokenResponse = {
6363
...TEST_ID_TOKEN_RESPONSE,
6464
oauthAccessToken: 'access-token'
6565
};

‎packages-exp/auth-exp/src/core/providers/facebook.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export class FacebookAuthProvider extends OAuthProvider {
4848
error: FirebaseError
4949
): externs.OAuthCredential | null {
5050
return FacebookAuthProvider.credentialFromTaggedObject(
51-
error as TaggedWithTokenResponse
51+
(error.customData || {}) as TaggedWithTokenResponse
5252
);
5353
}
5454

‎packages-exp/auth-exp/src/core/providers/github.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ describe('src/core/providers/github', () => {
5959
const error = AUTH_ERROR_FACTORY.create(AuthErrorCode.NEED_CONFIRMATION, {
6060
appName: 'foo'
6161
});
62-
(error as TaggedWithTokenResponse)._tokenResponse = {
62+
(error.customData! as TaggedWithTokenResponse)._tokenResponse = {
6363
...TEST_ID_TOKEN_RESPONSE,
6464
oauthAccessToken: 'access-token'
6565
};

‎packages-exp/auth-exp/src/core/providers/github.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export class GithubAuthProvider extends OAuthProvider {
4848
error: FirebaseError
4949
): externs.OAuthCredential | null {
5050
return GithubAuthProvider.credentialFromTaggedObject(
51-
error as TaggedWithTokenResponse
51+
(error.customData || {}) as TaggedWithTokenResponse
5252
);
5353
}
5454

‎packages-exp/auth-exp/src/core/providers/google.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ describe('src/core/providers/google', () => {
6262
const error = AUTH_ERROR_FACTORY.create(AuthErrorCode.NEED_CONFIRMATION, {
6363
appName: 'foo'
6464
});
65-
(error as TaggedWithTokenResponse)._tokenResponse = {
65+
(error.customData! as TaggedWithTokenResponse)._tokenResponse = {
6666
...TEST_ID_TOKEN_RESPONSE,
6767
oauthAccessToken: 'access-token',
6868
oauthIdToken: 'id-token'

‎packages-exp/auth-exp/src/core/providers/google.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ export class GoogleAuthProvider extends OAuthProvider {
5353
error: FirebaseError
5454
): externs.OAuthCredential | null {
5555
return GoogleAuthProvider.credentialFromTaggedObject(
56-
error as TaggedWithTokenResponse
56+
(error.customData || {}) as TaggedWithTokenResponse
5757
);
5858
}
5959

‎packages-exp/auth-exp/src/core/providers/twitter.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ describe('src/core/providers/twitter', () => {
7979
const error = AUTH_ERROR_FACTORY.create(AuthErrorCode.NEED_CONFIRMATION, {
8080
appName: 'foo'
8181
});
82-
(error as TaggedWithTokenResponse)._tokenResponse = {
82+
(error.customData! as TaggedWithTokenResponse)._tokenResponse = {
8383
...TEST_ID_TOKEN_RESPONSE,
8484
oauthAccessToken: 'access-token',
8585
oauthTokenSecret: 'token-secret'

‎packages-exp/auth-exp/src/core/providers/twitter.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ export class TwitterAuthProvider extends OAuthProvider {
6767
error: FirebaseError
6868
): externs.OAuthCredential | null {
6969
return TwitterAuthProvider.credentialFromTaggedObject(
70-
error as TaggedWithTokenResponse
70+
(error.customData || {}) as TaggedWithTokenResponse
7171
);
7272
}
7373

‎packages-exp/auth-exp/src/mfa/mfa_error.ts

+3-2
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,9 @@ export class MultiFactorError
4646
Object.setPrototypeOf(this, MultiFactorError.prototype);
4747
this.appName = auth.name;
4848
this.code = error.code;
49-
this.tenantid = auth.tenantId;
50-
this.serverResponse = error.serverResponse as IdTokenMfaResponse;
49+
this.tenantId = auth.tenantId ?? undefined;
50+
this.serverResponse = error.customData!
51+
.serverResponse as IdTokenMfaResponse;
5152
}
5253

5354
static _fromErrorAndCredential(

‎packages-exp/installations-exp/src/helpers/get-installation-entry.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ async function registerInstallation(
138138
);
139139
return set(appConfig, registeredInstallationEntry);
140140
} catch (e) {
141-
if (isServerError(e) && e.serverCode === 409) {
141+
if (isServerError(e) && e.customData.serverCode === 409) {
142142
// Server returned a "FID can not be used" error.
143143
// Generate a new ID next time.
144144
await remove(appConfig);

‎packages-exp/installations-exp/src/helpers/refresh-auth-token.ts

+4-1
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,10 @@ async function fetchAuthTokenFromServer(
150150
await set(installations.appConfig, updatedInstallationEntry);
151151
return authToken;
152152
} catch (e) {
153-
if (isServerError(e) && (e.serverCode === 401 || e.serverCode === 404)) {
153+
if (
154+
isServerError(e) &&
155+
(e.customData.serverCode === 401 || e.customData.serverCode === 404)
156+
) {
154157
// Server returned a "FID not found" or a "Invalid authentication" error.
155158
// Generate a new ID next time.
156159
await remove(installations.appConfig);

‎packages-exp/installations-exp/src/util/errors.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ export interface ServerErrorData {
6161
serverStatus: string;
6262
}
6363

64-
export type ServerError = FirebaseError & ServerErrorData;
64+
export type ServerError = FirebaseError & { customData: ServerErrorData };
6565

6666
/** Returns true if error is a FirebaseError that is based on an error from the server. */
6767
export function isServerError(error: unknown): error is ServerError {

‎packages/analytics/src/get-config.ts

+6-4
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ async function attemptFetchDynamicConfigWithRetry(
222222
}
223223

224224
const backoffMillis =
225-
Number(e.httpStatus) === 503
225+
Number(e.customData.httpStatus) === 503
226226
? calculateBackoffMillis(
227227
backoffCount,
228228
retryData.intervalMillis,
@@ -284,16 +284,18 @@ function setAbortableTimeout(
284284
});
285285
}
286286

287+
type RetriableError = FirebaseError & { customData: { httpStatus: string } };
288+
287289
/**
288290
* Returns true if the {@link Error} indicates a fetch request may succeed later.
289291
*/
290-
function isRetriableError(e: Error): boolean {
291-
if (!(e instanceof FirebaseError)) {
292+
function isRetriableError(e: Error): e is RetriableError {
293+
if (!(e instanceof FirebaseError) || !e.customData) {
292294
return false;
293295
}
294296

295297
// Uses string index defined by ErrorData, which FirebaseError implements.
296-
const httpStatus = Number(e['httpStatus']);
298+
const httpStatus = Number(e.customData['httpStatus']);
297299

298300
return (
299301
httpStatus === 429 ||

‎packages/installations/src/helpers/get-installation-entry.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ async function registerInstallation(
138138
);
139139
return set(appConfig, registeredInstallationEntry);
140140
} catch (e) {
141-
if (isServerError(e) && e.serverCode === 409) {
141+
if (isServerError(e) && e.customData.serverCode === 409) {
142142
// Server returned a "FID can not be used" error.
143143
// Generate a new ID next time.
144144
await remove(appConfig);

‎packages/installations/src/helpers/refresh-auth-token.ts

+4-1
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,10 @@ async function fetchAuthTokenFromServer(
148148
await set(dependencies.appConfig, updatedInstallationEntry);
149149
return authToken;
150150
} catch (e) {
151-
if (isServerError(e) && (e.serverCode === 401 || e.serverCode === 404)) {
151+
if (
152+
isServerError(e) &&
153+
(e.customData.serverCode === 401 || e.customData.serverCode === 404)
154+
) {
152155
// Server returned a "FID not found" or a "Invalid authentication" error.
153156
// Generate a new ID next time.
154157
await remove(dependencies.appConfig);

‎packages/installations/src/util/errors.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ export interface ServerErrorData {
6161
serverStatus: string;
6262
}
6363

64-
export type ServerError = FirebaseError & ServerErrorData;
64+
export type ServerError = FirebaseError & { customData: ServerErrorData };
6565

6666
/** Returns true if error is a FirebaseError that is based on an error from the server. */
6767
export function isServerError(error: unknown): error is ServerError {

‎packages/remote-config/src/client/retrying_client.ts

+4-3
Original file line numberDiff line numberDiff line change
@@ -61,16 +61,17 @@ export function setAbortableTimeout(
6161
});
6262
}
6363

64+
type RetriableError = FirebaseError & { customData: { httpStatus: string } };
6465
/**
6566
* Returns true if the {@link Error} indicates a fetch request may succeed later.
6667
*/
67-
function isRetriableError(e: Error): boolean {
68-
if (!(e instanceof FirebaseError)) {
68+
function isRetriableError(e: Error): e is RetriableError {
69+
if (!(e instanceof FirebaseError) || !e.customData) {
6970
return false;
7071
}
7172

7273
// Uses string index defined by ErrorData, which FirebaseError implements.
73-
const httpStatus = Number(e['httpStatus']);
74+
const httpStatus = Number(e.customData['httpStatus']);
7475

7576
return (
7677
httpStatus === 429 ||

‎packages/remote-config/test/client/rest_client.test.ts

+10-4
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,10 @@ describe('RestClient', () => {
134134

135135
await expect(fetchPromise)
136136
.to.eventually.be.rejectedWith(FirebaseError, firebaseError.message)
137-
.with.property('originalErrorMessage', 'Network request failed');
137+
.with.nested.property(
138+
'customData.originalErrorMessage',
139+
'Network request failed'
140+
);
138141
});
139142

140143
it('throws on JSON parse failure', async () => {
@@ -154,7 +157,10 @@ describe('RestClient', () => {
154157

155158
await expect(fetchPromise)
156159
.to.eventually.be.rejectedWith(FirebaseError, firebaseError.message)
157-
.with.property('originalErrorMessage', 'Unexpected end of input');
160+
.with.nested.property(
161+
'customData.originalErrorMessage',
162+
'Unexpected end of input'
163+
);
158164
});
159165

160166
it('handles 304 status code and empty body', async () => {
@@ -200,7 +206,7 @@ describe('RestClient', () => {
200206

201207
await expect(fetchPromise)
202208
.to.eventually.be.rejectedWith(FirebaseError, error.message)
203-
.with.property('httpStatus', 500);
209+
.with.nested.property('customData.httpStatus', 500);
204210
});
205211

206212
it('normalizes NO_CHANGE state to 304 status', async () => {
@@ -257,7 +263,7 @@ describe('RestClient', () => {
257263

258264
await expect(fetchPromise)
259265
.to.eventually.be.rejectedWith(FirebaseError, error.message)
260-
.with.property('httpStatus', status);
266+
.with.nested.property('customData.httpStatus', status);
261267
}
262268
});
263269
});

‎packages/util/src/errors.ts

+6-30
Original file line numberDiff line numberDiff line change
@@ -69,26 +69,16 @@ export interface ErrorData {
6969
[key: string]: unknown;
7070
}
7171

72-
export interface FirebaseError extends Error, ErrorData {
73-
// Unique code for error - format is service/error-code-string.
74-
readonly code: string;
75-
76-
// Developer-friendly error message.
77-
readonly message: string;
78-
79-
// Always 'FirebaseError'.
80-
readonly name: typeof ERROR_NAME;
81-
82-
// Where available - stack backtrace in a string.
83-
readonly stack?: string;
84-
}
85-
8672
// Based on code from:
8773
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Custom_Error_Types
8874
export class FirebaseError extends Error {
8975
readonly name = ERROR_NAME;
9076

91-
constructor(readonly code: string, message: string) {
77+
constructor(
78+
readonly code: string,
79+
message: string,
80+
public customData?: Record<string, unknown>
81+
) {
9282
super(message);
9383

9484
// Fix For ES5
@@ -125,21 +115,7 @@ export class ErrorFactory<
125115
// Service Name: Error message (service/code).
126116
const fullMessage = `${this.serviceName}: ${message} (${fullCode}).`;
127117

128-
const error = new FirebaseError(fullCode, fullMessage);
129-
130-
// Keys with an underscore at the end of their name are not included in
131-
// error.data for some reason.
132-
// TODO: Replace with Object.entries when lib is updated to es2017.
133-
for (const key of Object.keys(customData)) {
134-
if (key.slice(-1) !== '_') {
135-
if (key in error) {
136-
console.warn(
137-
`Overwriting FirebaseError base field "${key}" can cause unexpected behavior.`
138-
);
139-
}
140-
error[key] = customData[key];
141-
}
142-
}
118+
const error = new FirebaseError(fullCode, fullMessage, customData);
143119

144120
return error;
145121
}

‎packages/util/test/errors.test.ts

+1-24
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
* limitations under the License.
1616
*/
1717
import { assert } from 'chai';
18-
import { stub } from 'sinon';
1918
import { ErrorFactory, ErrorMap, FirebaseError } from '../src/errors';
2019

2120
type ErrorCode =
@@ -60,14 +59,7 @@ describe('FirebaseError', () => {
6059
e.message,
6160
"Fake: Could not find file: 'foo.txt' (fake/file-not-found)."
6261
);
63-
assert.equal(e.file, 'foo.txt');
64-
});
65-
66-
it('anonymously replaces template values with data', () => {
67-
const e = ERROR_FACTORY.create('anon-replace', { repl_: 'world' });
68-
assert.equal(e.code, 'fake/anon-replace');
69-
assert.equal(e.message, 'Fake: Hello, world! (fake/anon-replace).');
70-
assert.isUndefined(e.repl_);
62+
assert.equal(e.customData!.file, 'foo.txt');
7163
});
7264

7365
it('uses "Error" as template when template is missing', () => {
@@ -88,21 +80,6 @@ describe('FirebaseError', () => {
8880
);
8981
});
9082

91-
it('warns if overwriting a base error field with custom data', () => {
92-
const warnStub = stub(console, 'warn');
93-
const e = ERROR_FACTORY.create('overwrite-field', {
94-
code: 'overwritten code'
95-
});
96-
assert.equal(e.code, 'overwritten code');
97-
// TODO: use sinon-chai for this.
98-
assert.ok(
99-
warnStub.calledOnceWith(
100-
'Overwriting FirebaseError base field "code" can cause unexpected behavior.'
101-
)
102-
);
103-
warnStub.restore();
104-
});
105-
10683
it('has stack', () => {
10784
const e = ERROR_FACTORY.create('generic-error');
10885

0 commit comments

Comments
 (0)