Skip to content
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.

Sign up for GitHub

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

Jump to bottom

httpsCallable error code format change from v8 to v9 should be warned loudly #6281

Closed
qqpann opened this issue May 18, 2022 · 4 comments · Fixed by #6344
Closed

httpsCallable error code format change from v8 to v9 should be warned loudly #6281

qqpann opened this issue May 18, 2022 · 4 comments · Fixed by #6344
Labels
api: functions v9

Comments

@qqpann
Copy link

qqpann commented May 18, 2022 •
edited
Loading

[REQUIRED] Describe your environment

  • Operating System version: _____
  • Browser version: _____
  • Firebase SDK version: _____
  • Firebase Product: _____ (auth, database, storage, etc)

[REQUIRED] Describe the problem

v8:

firebase-js-sdk/packages/functions/test/callable.test.ts

Line 50 in 919413c

expect(e.code).to.equal(code);

v9:

firebase-js-sdk/packages/functions-compat/src/callable.test.ts

Line 40 in cdada6c

expect(e.code).to.match(new RegExp(`functions.*/${code}`));

The change of error code format (it attaches "functions/" at the beginning since v9) is not mentioned in the v8 v9 migration guide. It should be well announced in the documentation.
Otherwise, if a user relies on the error code string in their app, the upgrade will cause a crush (like me 😢 ).

Steps to reproduce:

Relevant Code:

// TODO(you): code here to reproduce the problem
@google-oss-bot
Copy link
Contributor

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

@ishowta
Copy link

ishowta commented May 18, 2022

On the other hand, error code in firestore does not have a prefix, which seems inconsistent with functions one.

@felixh10r
Copy link

What makes this problem worse is that the TypeScript definitions do not reflect the change and are currently different from actual runtime behavior. Adjusting the types would warn developers about the breaking change. Please tell me if I should open a PR.

https://github.com/firebase/firebase-js-sdk/blob/cdada6c68f9740d13dd6674bcb658e28e68253b6/packages/functions/src/public-types.ts#L115:L132

@hsubox76
Copy link
Contributor

hsubox76 commented Jun 6, 2022

That can be done, but we need to keep the non-prefixed type around as a param type for the FunctionsError constructor here:

firebase-js-sdk/packages/functions/src/error.ts

Line 61 in cdada6c

code: FunctionsErrorCode,

Seems like there's a fancy Typescript way of making a type that extends that type and adds the prefix, which can be used as the public type for FunctionsError. Feel free to make a PR or I will try to get to it when I can.

On the other hand, error code in firestore does not have a prefix, which seems inconsistent with functions one.

I think we try to wrap every error thrown by an SDK in the appropriate custom error wrapper for that product, but we may have missed some or not caught some errors that aren't thrown directly by the SDK code.

@hsubox76 hsubox76 mentioned this issue Jun 8, 2022
Merged
This was referenced Jul 6, 2022
Merged
Merged
@firebase firebase locked and limited conversation to collaborators Jul 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
api: functions v9
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add public FunctionsErrorCode type firebase/firebase-js-sdk
6 participants
@felixh10r @ishowta @hsubox76 @qqpann @google-oss-bot @jbalidiong