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

Change nativeErrorCode, nativeErrorMessage to be nullable #8206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jvliwanag
Copy link

Description

Update nativeErrorCode and nativeErrorMessage to reflect the possibility of having null values based here and here.

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

Copy link

vercel bot commented Dec 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 1, 2025 8:19am

@CLAassistant
Copy link

CLAassistant commented Dec 31, 2024

CLA assistant check
All committers have signed the CLA.

@mikehardy
Copy link
Collaborator

mikehardy commented Dec 31, 2024

Hmmm 🤔 no denying that they currently can be null in javascript vs the typescript definition

Upstream in firebase-js-sdk the equivalent class has code and message as non-null though https://github.com/firebase/firebase-js-sdk/blob/main/packages/util/src/errors.ts

They have a lot of machinery for making sure the error is associated with a service, and then the code can be specific (and type safe!) or just their Err.GENERIC type, with the message defaulting to a templated version of a specific error or just the string 'Error'

Moving at least closer to that by doing something similar to their GENERIC error code with perhaps something truly generic as a service/detail code combo like 'unknown/internal-error' as the default code vs null, and 'Error' as the default message vs null might start the process of us converging our errors so they may be handled more consistently for people using firebase-js-sdk on web and react-native-firebase on native in same codebase

What do you think?

@jvliwanag
Copy link
Author

This doesn't change code and message which have counterparts on firebase-js-sdk really should be non nullable. Only nativeErrorCode and nativeErrorMessage.

Copy link

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Jan 29, 2025
@mikehardy
Copy link
Collaborator

Sorry for the delay here - we're slowly coming to consensus on how to converge our types better with firebase-js-sdk

I think the answer here is:

  • alter our type NativeFirebaseError to extend FirebaseError
  • by default set the nativeErrorCode and nativeErrorMessage to 'unknown' and 'an unknown error occurred'
  • by default set the same into FirebaseError code and message

The alternative, to make these nullable is actually a breaking change for consumers as their build will fail if they're not handling the types - regardless if it is fixing a real bug that nulls could come through or not

So instead of doing a breaking change the wrong way - making things farther from upstream - better to make them closer to upstream so our errors fit into existing firebase-js-sdk error-handling patterns as a drop-in

@mikehardy mikehardy added type: bug New bug report tools: typings TypeScript / Flow type: enhancement Implements a new Feature and removed Stale labels Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Attention tools: typings TypeScript / Flow type: bug New bug report type: enhancement Implements a new Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants