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.

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

Firebase Admin 12.1.1 issue with error.ts #2571

Closed
Nushio opened this issue May 23, 2024 · 14 comments · Fixed by #2581
Closed

Firebase Admin 12.1.1 issue with error.ts #2571

Nushio opened this issue May 23, 2024 · 14 comments · Fixed by #2581
Assignees

Comments

@Nushio
Copy link

Nushio commented May 23, 2024

[READ] Step 1: Are you in the right place?

  • For issues related to the code in this repository file a Github issue.
  • If the issue pertains to Cloud Firestore, read the instructions in the "Firestore issue"
    template.
  • For general technical questions, post a question on StackOverflow
    with the firebase tag.
  • For general Firebase discussion, use the firebase-talk
    google group.
  • For help troubleshooting your application that does not fall under one
    of the above categories, reach out to the personalized
    Firebase support channel.

[REQUIRED] Step 2: Describe your environment

  • Operating System version: MacOSX
  • Firebase SDK version: 12.2.1
  • Firebase Product: logger
  • Node.js version: 18.18.2
  • NPM version: 9.8.1

[REQUIRED] Step 3: Describe the problem

Upgraded a (mature) codebase from 12.1.0 to 12.1.1, unit tests will no longer pass.

TypeError: Cannot read properties of undefined (reading 'message')

Narrowed down the issue to src/utils/error.ts line 62

If I add an optional to errorInfo?.message, and the tests pass now.

This file hasn't changed in over 3 years according to the github history, so the fact that it's failing now might be related to something introduced in 12.1.1, probably #2151

Steps to reproduce:

Upgrade to 12.1.1, and write a simple test using jest.

Note I am using pnpm, but the error also occurs if you use npm:

at FirebaseError.get message [as message] (../node_modules/.pnpm/firebase-admin@12.1.1/node_modules/firebase-admin/lib/utils/error.js:46:31)
at Array.forEach ()
at Array.forEach ()
at Array.forEach ()
at Object. (../node_modules/.pnpm/firebase-functions@5.0.1_firebase-admin@12.1.1/node_modules/firebase-functions/lib/common/providers/firestore.js:25:19)
at Object. (../node_modules/.pnpm/firebase-functions@5.0.1_firebase-admin@12.1.1/node_modules/firebase-functions/lib/v2/providers/firestore.js:32:21)
at Object. (../node_modules/.pnpm/firebase-functions@5.0.1_firebase-admin@12.1.1/node_modules/firebase-functions/lib/v2/index.js:56:19)

Relevant Code:

/** @returns The error message. */
public get message(): string {
return this.errorInfo.message;
}

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

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

@Nushio Nushio changed the title Firebase Admin 12.2.1 issue with logger Firebase Admin 12.2.1 issue with error.ts May 23, 2024
@Nushio Nushio changed the title Firebase Admin 12.2.1 issue with error.ts Firebase Admin 12.1.1 issue with error.ts May 23, 2024
@aangelisc
Copy link

We are also experiencing this and there doesn't seem to be a good option for mocking this to avoid the problem.

@jbaldassari
Copy link

jbaldassari commented May 27, 2024

Same issue here trying to mock firebase-admin with vitest with the module installed via pnpm:

Error: [vitest] There was an error when mocking a module. If you are using "vi.mock" factory, make sure there are no top level variables inside, since this call is hoisted to top of the file. Read more: https://vitest.dev/api/vi.html#vi-mock
Caused by: TypeError: Cannot read properties of undefined (reading 'message')
 ❯ FirebaseError.get message [as message] node_modules/.pnpm/registry.npmjs.org+firebase-admin@12.1.1/node_modules/firebase-admin/lib/utils/error.js:46:31

Looking at the commits between 12.1.0 and 12.1.1, there were a couple of changes related to error classes, but nothing stands out to me as an obvious cause:

@Nushio
Copy link
Author

Nushio commented May 28, 2024

Glad I'm not the only one. I didn't find an obvious culprit but making the message part optional does allow mocking to work, which seems to me like some firebase error classes are instanciated without a defined message error, like

export class FirebaseDatabaseError extends FirebaseError {
constructor(info: ErrorInfo, message?: string) {

@lahirumaramba
Copy link
Member

lahirumaramba commented May 28, 2024

Thanks for reporting this. Does optional chaining (see below) fix the issue for you? If that's the case then I can prepare a fix in the SDK.

public get message(): string {

  /** @returns The error code. */
  public get code(): string {
    return this.errorInfo?.code;
  }

  /** @returns The error message. */
  public get message(): string {
    return this.errorInfo?.message;
  }

@Nushio
Copy link
Author

Nushio commented May 28, 2024

It does for me. That's how I fixed it internally.

@lahirumaramba
Copy link
Member

#2581 should fix it, but if you can provide us with a minimal repro that would be very useful for us to investigate and confirm the fix. Thanks!

@jbaldassari
Copy link

jbaldassari commented May 28, 2024

Thanks @lahirumaramba . Here's a minimal reproduction case with 12.1.1: https://stackblitz.com/edit/firebase-admin-node-2571?file=test%2Fbasic.test.ts

image

If you edit package.json, change the firebase-admin version to 12.1.0, then reload, the test should pass.

@Nushio
Copy link
Author

Nushio commented May 28, 2024

I also tested the PR with our project and it works fine!

@lahirumaramba
Copy link
Member

lahirumaramba commented May 29, 2024

Thanks @jbaldassari for the repro! I really appreciate it.
Thanks @Nushio for confirming the fix. We will include this in the release next week.

@Nushio
Copy link
Author

Nushio commented Jun 14, 2024

Hi @lahirumaramba , any eta for the release? We've been holding out from upgrading to 12.1.1 for about 2 weeks now.

@lahirumaramba
Copy link
Member

We will include this in the release next week. Thanks for your patience!

@lahirumaramba
Copy link
Member

The fix is now released in v12.2.0. Let us know if you run into any issues!

@Nushio
Copy link
Author

Nushio commented Jun 20, 2024

Thank you! I'm deploying to staging, and will probably push this onto prod on Monday. We've been really looking forward the updated firestore library to see if it stabilizes our issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants